|
|
Created:
3 years, 8 months ago by Mark P Modified:
3 years, 8 months ago CC:
chromium-reviews, jdonnelly+watch_chromium.org, ios-reviews_chromium.org, droger+watchlist_chromium.org, blundell+watchlist_chromium.org, sdefresne+watchlist_chromium.org, dcheng, mac-reviews_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionOmnibox - Reduce Clipboard URL Suggestions to 1 Hour
and make it experiment-controlled in case we want to change it later.
TBR=jif
(to make this gets in before the branch point; please let me know your
feedback so I can make necessary revisions later)
BUG=709634
Review-Url: https://codereview.chromium.org/2808413003
Cr-Commit-Position: refs/heads/master@{#464532}
Committed: https://chromium.googlesource.com/chromium/src/+/a78996284e826673ba36113865a623743c7ef048
Patch Set 1 #Patch Set 2 : cleanup field trial.h #
Total comments: 4
Patch Set 3 : pkasting's comments #Patch Set 4 : rebase #
Total comments: 2
Patch Set 5 : fix mac #Patch Set 6 : revise include #Patch Set 7 : rebase #Patch Set 8 : fix rebase error #
Messages
Total messages: 35 (25 generated)
Description was changed from ========== Omnibox - Reduce Clipboard URL Suggestions to 1 Hour and make it experiment-controlled. BUG=709634 ========== to ========== Omnibox - Reduce Clipboard URL Suggestions to 1 Hour and make it experiment-controlled in case we want to change it later. BUG=709634 ==========
mpearson@chromium.org changed reviewers: + jif@chromium.org
jif@ Could you please take a look at this change? For reference, the looking up a feature parameter and convert to int is exactly the same code used in many places in omnibox_field_trial.cc. thanks, mark
FYI, jif is away to next Tuesday.
mpearson@chromium.org changed reviewers: + pkasting@chromium.org - jif@chromium.org
Peter, Can you please be the primary reviewer for this changelist? jif@ is away, and this change should get in this week. It's a pretty straightforward change and you're already familiar with some of this code. thanks, mark
LGTM https://codereview.chromium.org/2808413003/diff/20001/components/omnibox/brow... File components/omnibox/browser/omnibox_field_trial.h (right): https://codereview.chromium.org/2808413003/diff/20001/components/omnibox/brow... components/omnibox/browser/omnibox_field_trial.h:431: // One parameter for the omnibox field trial that doesn't live in this file; Nit: Remove "that" https://codereview.chromium.org/2808413003/diff/20001/components/omnibox/brow... components/omnibox/browser/omnibox_field_trial.h:437: // "ClipboardURLMaximumAge". Nit: Name the parameter at the beginning of the comment, then say why it can't live here. (This is sort of the paragraph structure version of "don't use passive voice".)
https://codereview.chromium.org/2808413003/diff/20001/components/omnibox/brow... File components/omnibox/browser/omnibox_field_trial.h (right): https://codereview.chromium.org/2808413003/diff/20001/components/omnibox/brow... components/omnibox/browser/omnibox_field_trial.h:431: // One parameter for the omnibox field trial that doesn't live in this file; On 2017/04/12 20:02:30, Peter Kasting wrote: > Nit: Remove "that" Done. https://codereview.chromium.org/2808413003/diff/20001/components/omnibox/brow... components/omnibox/browser/omnibox_field_trial.h:437: // "ClipboardURLMaximumAge". On 2017/04/12 20:02:30, Peter Kasting wrote: > Nit: Name the parameter at the beginning of the comment, then say why it can't > live here. (This is sort of the paragraph structure version of "don't use > passive voice".) Reorganized and rewrote. Take a gander.
Description was changed from ========== Omnibox - Reduce Clipboard URL Suggestions to 1 Hour and make it experiment-controlled in case we want to change it later. BUG=709634 ========== to ========== Omnibox - Reduce Clipboard URL Suggestions to 1 Hour and make it experiment-controlled in case we want to change it later. TBR=jif (to make this gets in before the branch point; please let me know your feedback so I can make necessary revisions later) BUG=709634 ==========
The CQ bit was checked by mpearson@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...
https://codereview.chromium.org/2808413003/diff/60001/components/omnibox/brow... File components/omnibox/browser/omnibox_field_trial.h (right): https://codereview.chromium.org/2808413003/diff/60001/components/omnibox/brow... components/omnibox/browser/omnibox_field_trial.h:436: // include this component, else there would be a circular dependency. Cool, this is better. Although reading it started to make me wonder "maybe we should just leave this file untouched and not say anything", but whatever.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
https://codereview.chromium.org/2808413003/diff/60001/components/omnibox/brow... File components/omnibox/browser/omnibox_field_trial.h (right): https://codereview.chromium.org/2808413003/diff/60001/components/omnibox/brow... components/omnibox/browser/omnibox_field_trial.h:436: // include this component, else there would be a circular dependency. On 2017/04/12 22:49:49, Peter Kasting wrote: > Cool, this is better. > > Although reading it started to make me wonder "maybe we should just leave this > file untouched and not say anything", but whatever. I think it's good to have a central place where people can find about all existing omnibox experiments (and be able to clean them up later too).
The CQ bit was checked by mpearson@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: Try jobs failed on following builders: ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
The CQ bit was checked by mpearson@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: Try jobs failed on following builders: ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
The CQ bit was checked by mpearson@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.
The CQ bit was checked by mpearson@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pkasting@chromium.org Link to the patchset: https://codereview.chromium.org/2808413003/#ps140001 (title: "fix rebase error")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 140001, "attempt_start_ts": 1492115603114360, "parent_rev": "e03b7960f6c5e53e3b84ef06c99fda5ad500389d", "commit_rev": "a78996284e826673ba36113865a623743c7ef048"}
Message was sent while issue was closed.
Description was changed from ========== Omnibox - Reduce Clipboard URL Suggestions to 1 Hour and make it experiment-controlled in case we want to change it later. TBR=jif (to make this gets in before the branch point; please let me know your feedback so I can make necessary revisions later) BUG=709634 ========== to ========== Omnibox - Reduce Clipboard URL Suggestions to 1 Hour and make it experiment-controlled in case we want to change it later. TBR=jif (to make this gets in before the branch point; please let me know your feedback so I can make necessary revisions later) BUG=709634 Review-Url: https://codereview.chromium.org/2808413003 Cr-Commit-Position: refs/heads/master@{#464532} Committed: https://chromium.googlesource.com/chromium/src/+/a78996284e826673ba36113865a6... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as https://chromium.googlesource.com/chromium/src/+/a78996284e826673ba36113865a6...
Message was sent while issue was closed.
jif@chromium.org changed reviewers: + jif@chromium.org
Message was sent while issue was closed.
since lody@ refactored the iOS code: lgtm |