Omnibox - Cleanup From Recent Changelists

Omnibox - Cleanup From Recent Changelists

Includes code polish requested mainly on
https://codereview.chromium.org/2717893002/
and one request on
https://codereview.chromium.org/2726413002/

BUG=

Review-Url: https://codereview.chromium.org/2725333004
Cr-Commit-Position: refs/heads/master@{#454720}
Committed: https://chromium.googlesource.com/chromium/src/+/c90c24bbb9e18ea9e90e34fa2ed975435be403c3

[Mark P, 2017-03-03 22:11:52]

Description was changed from

==========
Omnibox - Cleanup From Recent Changelists

Includes code polish requested mainly on
https://codereview.chromium.org/2724303002/
and one request on
https://codereview.chromium.org/2726413002/

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

to

==========
Omnibox - Cleanup From Recent Changelists

Includes code polish requested mainly on
https://codereview.chromium.org/2717893002/
and one request on
https://codereview.chromium.org/2726413002/

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

[Mark P, 2017-03-03 22:13:14]

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

[Mark P, 2017-03-03 22:13:27]

Feel free to review this at your leisure.

cheers,
mark

[Mark P, 2017-03-03 22:13:37]

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

[commit-bot: I haz the power, 2017-03-03 22:13:59]

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-03-03 22:37:53]

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

[commit-bot: I haz the power, 2017-03-03 22:37:54]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Peter Kasting, 2017-03-03 23:13:46]

https://codereview.chromium.org/2725333004/diff/1/components/omnibox/browser/...
File components/omnibox/browser/search_provider.cc (right):

https://codereview.chromium.org/2725333004/diff/1/components/omnibox/browser/...
components/omnibox/browser/search_provider.cc:276: DCHECK(keyword_fetcher_ !=
nullptr);
This seems backwards.  I'd think we'd want to check that these _are_ null, not
that they're not.

But would it not be sufficient to just check |done_|?

[Mark P, 2017-03-03 23:22:26]

https://codereview.chromium.org/2725333004/diff/1/components/omnibox/browser/...
File components/omnibox/browser/search_provider.cc (right):

https://codereview.chromium.org/2725333004/diff/1/components/omnibox/browser/...
components/omnibox/browser/search_provider.cc:276: DCHECK(keyword_fetcher_ !=
nullptr);
On 2017/03/03 23:13:46, Peter Kasting wrote:
> This seems backwards.  I'd think we'd want to check that these _are_ null, not
> that they're not.

Heh, no wonder I just got an e-mail that the dry run failed. :-)

> But would it not be sufficient to just check |done_|?

Yes instead.  Fixed.

[Mark P, 2017-03-03 23:22:31]

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

[commit-bot: I haz the power, 2017-03-03 23:22:54]

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-03-03 23:27:44]

LGTM

[commit-bot: I haz the power, 2017-03-04 00:03:09]

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

[commit-bot: I haz the power, 2017-03-04 00:03:10]

Dry run: This issue passed the CQ dry run.

[Mark P, 2017-03-04 00:07:24]

The CQ bit was checked by mpearson@chromium.org

[commit-bot: I haz the power, 2017-03-04 00:07:42]

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-03-04 00:11:35]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1488586044219040,
"parent_rev": "579928641781a4b681aa572b5f3bb3b5fac0137c", "commit_rev":
"c90c24bbb9e18ea9e90e34fa2ed975435be403c3"}

[commit-bot: I haz the power, 2017-03-04 00:12:14]

Description was changed from

==========
Omnibox - Cleanup From Recent Changelists

Includes code polish requested mainly on
https://codereview.chromium.org/2717893002/
and one request on
https://codereview.chromium.org/2726413002/

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

to

==========
Omnibox - Cleanup From Recent Changelists

Includes code polish requested mainly on
https://codereview.chromium.org/2717893002/
and one request on
https://codereview.chromium.org/2726413002/

BUG=

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

[commit-bot: I haz the power, 2017-03-04 00:12:15]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/c90c24bbb9e18ea9e90e34fa2ed9...