Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2187033002:
[Offline Pages] Make querying by client ID support a list. (Closed)
|
Created:
4 years, 4 months ago by dewittj Modified:
4 years, 4 months ago Reviewers:
dougarnett CC:
chromium-reviews, romax+watch_chromium.org, fgorski+watch_chromium.org, dewittj+watch_chromium.org, petewil+watch_chromium.org, chili+watch_chromium.org, dimich+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Offline Pages] Make querying by client ID support a list.
This is an inefficient intermediate step to support multiple client IDs
in a single API call. TODO: update native backend to support multiple
Client ID search.
BUG=
Committed: https://crrev.com/94274706ed2b9c2e3f8f22400649ad047f221f6e
Cr-Commit-Position: refs/heads/master@{#408785}
Patch Set 1 #
Total comments: 4
Patch Set 2 : nits #
Messages
Total messages: 20 (13 generated)
The CQ bit was checked by dewittj@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== [Offline Pages] Make querying by client ID support a list. This is an inefficient intermediate step to support multiple client IDs in a single API call. TODO: update native backend to support multiple Client ID search. BUG= ========== to ========== [Offline Pages] Make querying by client ID support a list. This is an inefficient intermediate step to support multiple client IDs in a single API call. TODO: update native backend to support multiple Client ID search. BUG= ==========
dewittj@chromium.org changed reviewers: + dougarnett@chromium.org
https://codereview.chromium.org/2187033002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridge.java (right): https://codereview.chromium.org/2187033002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridge.java:214: * Gets the offline pages associated with a provided client ID. please update javadoc https://codereview.chromium.org/2187033002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridge.java:239: // TODO(dewittj): Restructure the native API to avoid this loop with a native call. Just question: So there is an existing native call loop around offline ids and then we are adding a loop around this on client ids. Are you wanting to later just get rid of inner loop or both loops?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2187033002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridge.java (right): https://codereview.chromium.org/2187033002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridge.java:214: * Gets the offline pages associated with a provided client ID. On 2016/07/27 17:40:59, dougarnett wrote: > please update javadoc Done. https://codereview.chromium.org/2187033002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridge.java:239: // TODO(dewittj): Restructure the native API to avoid this loop with a native call. On 2016/07/27 17:40:59, dougarnett wrote: > Just question: So there is an existing native call loop around offline ids and > then we are adding a loop around this on client ids. Are you wanting to later > just get rid of inner loop or both loops? I think that ideally getPagesByClientIdInternal would disappear completely.
The CQ bit was checked by dewittj@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
The CQ bit was checked by dewittj@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== [Offline Pages] Make querying by client ID support a list. This is an inefficient intermediate step to support multiple client IDs in a single API call. TODO: update native backend to support multiple Client ID search. BUG= ========== to ========== [Offline Pages] Make querying by client ID support a list. This is an inefficient intermediate step to support multiple client IDs in a single API call. TODO: update native backend to support multiple Client ID search. BUG= ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== [Offline Pages] Make querying by client ID support a list. This is an inefficient intermediate step to support multiple client IDs in a single API call. TODO: update native backend to support multiple Client ID search. BUG= ========== to ========== [Offline Pages] Make querying by client ID support a list. This is an inefficient intermediate step to support multiple client IDs in a single API call. TODO: update native backend to support multiple Client ID search. BUG= Committed: https://crrev.com/94274706ed2b9c2e3f8f22400649ad047f221f6e Cr-Commit-Position: refs/heads/master@{#408785} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/94274706ed2b9c2e3f8f22400649ad047f221f6e Cr-Commit-Position: refs/heads/master@{#408785} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
