Omnibox - Reduce Clipboard URL Suggestions to 1 Hour

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/+/a78996284e826673ba36113865a623743c7ef048

[Mark P, 2017-04-11 22:29:40]

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
==========

[Mark P, 2017-04-11 23:14:48]

mpearson@chromium.org changed reviewers:
+ jif@chromium.org

[Mark P, 2017-04-11 23:14:49]

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

[sdefresne, 2017-04-12 09:17:58]

FYI, jif is away to next Tuesday.

[Mark P, 2017-04-12 19:27:54]

mpearson@chromium.org changed reviewers:
+ pkasting@chromium.org
- jif@chromium.org

[Mark P, 2017-04-12 19:27:55]

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

[Peter Kasting, 2017-04-12 20:02:30]

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".)

[Mark P, 2017-04-12 21:56:59]

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.

[Mark P, 2017-04-12 21:58:37]

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
==========

[Mark P, 2017-04-12 22:37:49]

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

[commit-bot: I haz the power, 2017-04-12 22:39:02]

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

[Peter Kasting, 2017-04-12 22:49:49]

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.

[commit-bot: I haz the power, 2017-04-12 22:51:05]

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

[commit-bot: I haz the power, 2017-04-12 22:51:06]

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...)

[Mark P, 2017-04-12 23:08:15]

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).

[Mark P, 2017-04-12 23:18:59]

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

[commit-bot: I haz the power, 2017-04-12 23:19:25]

Dry run: 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, 2017-04-12 23:29:10]

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

[commit-bot: I haz the power, 2017-04-12 23:29:11]

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...)

[Mark P, 2017-04-12 23:52:47]

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

[commit-bot: I haz the power, 2017-04-12 23:53:38]

Dry run: 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, 2017-04-13 00:08:05]

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

[commit-bot: I haz the power, 2017-04-13 00:08:06]

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-...)

[Mark P, 2017-04-13 19:11:07]

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

[commit-bot: I haz the power, 2017-04-13 19:11:56]

Dry run: 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, 2017-04-13 20:19:24]

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

[commit-bot: I haz the power, 2017-04-13 20:19:25]

Dry run: This issue passed the CQ dry run.

[Mark P, 2017-04-13 20:33:23]

The CQ bit was checked by mpearson@chromium.org

[Mark P, 2017-04-13 20:33:23]

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")

[commit-bot: I haz the power, 2017-04-13 20:33:59]

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, 2017-04-13 20:39:49]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1492115603114360,
"parent_rev": "e03b7960f6c5e53e3b84ef06c99fda5ad500389d", "commit_rev":
"a78996284e826673ba36113865a623743c7ef048"}

[commit-bot: I haz the power, 2017-04-13 20:40:42]

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...
==========

[commit-bot: I haz the power, 2017-04-13 20:40:43]

Committed patchset #8 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/a78996284e826673ba36113865a6...

[jif, 2017-04-18 12:36:41]

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

[jif, 2017-04-18 12:36:42]

since lody@ refactored the iOS code: lgtm