Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2271283002:
Add request type to FetchSuggestionsRequest. (Closed)
DescriptionAdd request type to FetchSuggestionsRequest.
There are two types of requests: interactive (e.g. a fetch when NTP is
opened) and noninteractive (e.g. a background prefetch). The
noninteractive requests are less important and thus they can be dropped
first if needed. This CL adds request type to the request.
BUG=638224
Committed: https://crrev.com/2a444eb2c8fd642fb23bc2a1396bec823d9a515d
Cr-Commit-Position: refs/heads/master@{#414672}
Patch Set 1 : Addressed comments. #
Total comments: 4
Patch Set 2 : Addressed nit. #Patch Set 3 : Reflected server change. #Patch Set 4 : Rebase. #
Messages
Total messages: 35 (16 generated)
vitaliii@chromium.org changed reviewers: + treib@chromium.org
Hi, please have a look.
sfiera@chromium.org changed reviewers: + sfiera@chromium.org
https://codereview.chromium.org/2271283002/diff/1/components/ntp_snippets/ntp... File components/ntp_snippets/ntp_snippets_fetcher.cc (right): https://codereview.chromium.org/2271283002/diff/1/components/ntp_snippets/ntp... components/ntp_snippets/ntp_snippets_fetcher.cc:73: // TODO(vitaliii): Decide on actual values. See go/request-qos-2.0 for the Do we want to actually expose any of these values here? I would rather define our own enum in our API corresponding to what we actually know in the client (i.e. INTERACTIVE, BACKGROUND) and map those to specific behaviors in the backend. Keep in mind that go/ links are Google-internal. Chrome's public and my impression is that we don't use them much.
https://codereview.chromium.org/2271283002/diff/1/components/ntp_snippets/ntp... File components/ntp_snippets/ntp_snippets_fetcher.cc (right): https://codereview.chromium.org/2271283002/diff/1/components/ntp_snippets/ntp... components/ntp_snippets/ntp_snippets_fetcher.cc:73: // TODO(vitaliii): Decide on actual values. See go/request-qos-2.0 for the On 2016/08/24 17:54:54, sfiera wrote: > Do we want to actually expose any of these values here? I would rather define > our own enum in our API corresponding to what we actually know in the client > (i.e. INTERACTIVE, BACKGROUND) and map those to specific behaviors in the > backend. > > Keep in mind that go/ links are Google-internal. Chrome's public and my > impression is that we don't use them much. +1 - no go/ links please, and I agree that we should have a custom enum in the protocol instead of these values. https://codereview.chromium.org/2271283002/diff/1/components/ntp_snippets/ntp... File components/ntp_snippets/ntp_snippets_fetcher.h (right): https://codereview.chromium.org/2271283002/diff/1/components/ntp_snippets/ntp... components/ntp_snippets/ntp_snippets_fetcher.h:229: // Is the request user initiated? nit: empty line before please - this is different from the two params above; those are constant, but this is one per-request. https://codereview.chromium.org/2271283002/diff/1/components/ntp_snippets/ntp... components/ntp_snippets/ntp_snippets_fetcher.h:230: bool interactive_request_; Initialize this in the constructor.
Patchset #1 (id:1) has been deleted
Description was changed from ========== Add RequestQoS criticality to FetchSuggestionsRequest. There are two types of requests: interactive (e.g. a fetch when NTP is opened) and noninteractive (e.g. a background prefetch). The noninteractive requests are less important and thus they can be dropped first if needed. This CL adds this information to the request by adding RequestQoS.criticality. BUG=638224 ========== to ========== Add request type to FetchSuggestionsRequest. There are two types of requests: interactive (e.g. a fetch when NTP is opened) and noninteractive (e.g. a background prefetch). The noninteractive requests are less important and thus they can be dropped first if needed. This CL adds request type to the request. BUG=638224 ==========
Hi, please have a look.
LGTM with one more nit, but please wait for sfiera's approval too wrt the protocol. https://codereview.chromium.org/2271283002/diff/20001/components/ntp_snippets... File components/ntp_snippets/ntp_snippets_fetcher.cc (right): https://codereview.chromium.org/2271283002/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_fetcher.cc:264: count_to_fetch() {} Also initialize interactive_request here https://codereview.chromium.org/2271283002/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_fetcher.cc:582: << "): " << last_fetch_json_; Was this auto-formatted?! (It's fine either way, I'm just curious)
LGTM, but please submit the backend change and let it roll out to alpha first (should take 2-3h).
LGTM but you'll probably need a rebase, there's been like 20 changes to this file today.
The CQ bit was checked by vitaliii@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from treib@chromium.org Link to the patchset: https://codereview.chromium.org/2271283002/#ps60001 (title: "Reflected server change.")
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
Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by vitaliii@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from treib@chromium.org, sfiera@chromium.org Link to the patchset: https://codereview.chromium.org/2271283002/#ps80001 (title: "Rebase.")
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
Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by vitaliii@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
tschumann@chromium.org changed reviewers: + tschumann@chromium.org
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by vitaliii@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 ========== Add request type to FetchSuggestionsRequest. There are two types of requests: interactive (e.g. a fetch when NTP is opened) and noninteractive (e.g. a background prefetch). The noninteractive requests are less important and thus they can be dropped first if needed. This CL adds request type to the request. BUG=638224 ========== to ========== Add request type to FetchSuggestionsRequest. There are two types of requests: interactive (e.g. a fetch when NTP is opened) and noninteractive (e.g. a background prefetch). The noninteractive requests are less important and thus they can be dropped first if needed. This CL adds request type to the request. BUG=638224 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Add request type to FetchSuggestionsRequest. There are two types of requests: interactive (e.g. a fetch when NTP is opened) and noninteractive (e.g. a background prefetch). The noninteractive requests are less important and thus they can be dropped first if needed. This CL adds request type to the request. BUG=638224 ========== to ========== Add request type to FetchSuggestionsRequest. There are two types of requests: interactive (e.g. a fetch when NTP is opened) and noninteractive (e.g. a background prefetch). The noninteractive requests are less important and thus they can be dropped first if needed. This CL adds request type to the request. BUG=638224 Committed: https://crrev.com/2a444eb2c8fd642fb23bc2a1396bec823d9a515d Cr-Commit-Position: refs/heads/master@{#414672} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/2a444eb2c8fd642fb23bc2a1396bec823d9a515d Cr-Commit-Position: refs/heads/master@{#414672}
Message was sent while issue was closed.
https://codereview.chromium.org/2271283002/diff/20001/components/ntp_snippets... File components/ntp_snippets/ntp_snippets_fetcher.cc (right): https://codereview.chromium.org/2271283002/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_fetcher.cc:264: count_to_fetch() {} On 2016/08/25 09:26:58, Marc Treib wrote: > Also initialize interactive_request here Done. https://codereview.chromium.org/2271283002/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_fetcher.cc:582: << "): " << last_fetch_json_; On 2016/08/25 09:26:58, Marc Treib wrote: > Was this auto-formatted?! (It's fine either way, I'm just curious) Yes. Eclipse format happened to differ from git cl format. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
