Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(181)

Issue 2078021: First pass at experimental omnibox API. There are plenty of rough edges and (Closed)

Created:
10 years, 7 months ago by Matt Perry
Modified:
9 years, 7 months ago
CC:
chromium-reviews
Visibility:
Public.

Description

First pass at experimental omnibox API. There are plenty of rough edges and features to work on, but it's in a usable state. When an extension is installed that specifies an omnibox keyword in its manifest, we add that keyword to the user's list of Search Engines. The user can then edit this keyword later. I'm leveraging most of the original search engine keyword code. An extension keyword has a special URL that identifies it as an extension keyword. There is some special case code to treat these keywords slightly differently throughout the omnibox code. BUG=38884 Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=48503

Patch Set 1 #

Total comments: 54

Patch Set 2 : review feedback #

Total comments: 23

Patch Set 3 : api test #

Patch Set 4 : peter feedback #

Total comments: 7

Patch Set 5 : round 4 #

Patch Set 6 : minimal_changes #

Patch Set 7 : tweak to fix a test #

Patch Set 8 : saved matches #

Total comments: 4

Patch Set 9 : no prefer_keyword #

Unified diffs Side-by-side diffs Delta from patch set Stats (+801 lines, -99 lines) Patch
M chrome/app/generated_resources.grd View 1 2 3 4 5 6 3 chunks +15 lines, -0 lines 0 comments Download
M chrome/browser/autocomplete/autocomplete_edit.cc View 1 2 3 chunks +19 lines, -3 lines 0 comments Download
M chrome/browser/autocomplete/keyword_provider.h View 1 2 3 4 5 6 7 4 chunks +28 lines, -3 lines 0 comments Download
M chrome/browser/autocomplete/keyword_provider.cc View 1 2 3 4 5 6 7 8 11 chunks +128 lines, -19 lines 0 comments Download
M chrome/browser/cocoa/location_bar_view_mac.h View 1 chunk +1 line, -0 lines 0 comments Download
M chrome/browser/cocoa/location_bar_view_mac.mm View 1 5 chunks +12 lines, -6 lines 0 comments Download
M chrome/browser/cocoa/location_bar_view_mac_unittest.mm View 1 3 chunks +9 lines, -6 lines 0 comments Download
M chrome/browser/extensions/extension_function_dispatcher.cc View 1 2 chunks +4 lines, -0 lines 0 comments Download
M chrome/browser/extensions/extension_message_service.h View 1 chunk +3 lines, -0 lines 0 comments Download
M chrome/browser/extensions/extension_message_service.cc View 1 chunk +6 lines, -0 lines 0 comments Download
A chrome/browser/extensions/extension_omnibox_api.h View 1 2 3 4 1 chunk +39 lines, -0 lines 0 comments Download
A chrome/browser/extensions/extension_omnibox_api.cc View 1 2 3 4 1 chunk +67 lines, -0 lines 0 comments Download
A chrome/browser/extensions/extension_omnibox_apitest.cc View 1 chunk +134 lines, -0 lines 0 comments Download
M chrome/browser/extensions/extensions_service.h View 1 1 chunk +3 lines, -2 lines 0 comments Download
M chrome/browser/extensions/extensions_service.cc View 1 5 chunks +13 lines, -3 lines 0 comments Download
M chrome/browser/gtk/location_bar_view_gtk.cc View 1 3 chunks +13 lines, -19 lines 0 comments Download
M chrome/browser/search_engines/template_url.h View 1 2 chunks +4 lines, -1 line 0 comments Download
M chrome/browser/search_engines/template_url.cc View 1 3 chunks +11 lines, -1 line 0 comments Download
M chrome/browser/search_engines/template_url_model.h View 1 5 chunks +25 lines, -1 line 0 comments Download
M chrome/browser/search_engines/template_url_model.cc View 1 7 chunks +74 lines, -8 lines 0 comments Download
M chrome/browser/search_engines/template_url_table_model.cc View 1 chunk +2 lines, -1 line 0 comments Download
M chrome/browser/views/location_bar/keyword_hint_view.h View 1 chunk +0 lines, -4 lines 0 comments Download
M chrome/browser/views/location_bar/keyword_hint_view.cc View 2 chunks +6 lines, -16 lines 0 comments Download
M chrome/browser/views/location_bar/selected_keyword_view.cc View 2 chunks +8 lines, -5 lines 0 comments Download
M chrome/chrome_browser.gypi View 1 2 3 4 5 6 1 chunk +2 lines, -0 lines 0 comments Download
M chrome/chrome_tests.gypi View 3 4 5 6 7 1 chunk +1 line, -0 lines 0 comments Download
M chrome/common/extensions/api/extension_api.json View 1 2 1 chunk +67 lines, -0 lines 0 comments Download
A chrome/common/extensions/docs/examples/api/omnibox/background.html View 1 chunk +19 lines, -0 lines 0 comments Download
A chrome/common/extensions/docs/examples/api/omnibox/manifest.json View 1 chunk +7 lines, -0 lines 0 comments Download
M chrome/common/extensions/docs/experimental.html View 1 2 1 chunk +1 line, -0 lines 0 comments Download
M chrome/common/extensions/extension.h View 1 2 chunks +5 lines, -0 lines 0 comments Download
M chrome/common/extensions/extension.cc View 1 1 chunk +12 lines, -0 lines 0 comments Download
M chrome/common/extensions/extension_constants.h View 2 chunks +3 lines, -0 lines 0 comments Download
M chrome/common/extensions/extension_constants.cc View 2 chunks +6 lines, -0 lines 0 comments Download
M chrome/common/notification_type.h View 1 2 3 1 chunk +5 lines, -0 lines 0 comments Download
M chrome/renderer/resources/extension_apitest.js View 1 chunk +1 line, -1 line 0 comments Download
M chrome/renderer/resources/extension_process_bindings.js View 1 2 chunks +16 lines, -0 lines 0 comments Download
M chrome/renderer/resources/renderer_extension_bindings.js View 1 chunk +1 line, -0 lines 0 comments Download
A chrome/test/data/extensions/api_test/omnibox/manifest.json View 1 chunk +8 lines, -0 lines 0 comments Download
A chrome/test/data/extensions/api_test/omnibox/test.html View 1 chunk +23 lines, -0 lines 0 comments Download

Messages

Total messages: 21 (0 generated)
Matt Perry
Peter: please review the omnibox changes Aaron: extension code
10 years, 7 months ago (2010-05-19 23:29:45 UTC) #1
Peter Kasting
Despite the number of comments below, much of what's here looks OK to me. My ...
10 years, 7 months ago (2010-05-20 00:19:31 UTC) #2
Aaron Boodman
Neat. Some high level comments: - There are no tests. - You should add a ...
10 years, 7 months ago (2010-05-21 18:53:26 UTC) #3
Peter Kasting
On 2010/05/21 18:53:26, Aaron Boodman wrote: > If you really want to associate suggestions with ...
10 years, 7 months ago (2010-05-21 20:51:56 UTC) #4
Aaron Boodman
On 2010/05/21 20:51:56, Peter Kasting wrote: > I don't think that's a good idea by ...
10 years, 7 months ago (2010-05-21 23:02:40 UTC) #5
Peter Kasting
On Fri, May 21, 2010 at 4:02 PM, <aa@chromium.org> wrote: > I think we might ...
10 years, 7 months ago (2010-05-21 23:08:25 UTC) #6
Matt Perry
Responses inline. As for the onInputChanged event, let me try to convince you that my ...
10 years, 7 months ago (2010-05-25 21:07:13 UTC) #7
Matt Perry
Oh, and I'll work on adding some tests.
10 years, 7 months ago (2010-05-25 21:10:07 UTC) #8
Aaron Boodman
On Tue, May 25, 2010 at 2:07 PM, <mpcomplete@chromium.org> wrote: > Responses inline. > > ...
10 years, 7 months ago (2010-05-25 22:06:50 UTC) #9
Matt Perry
On 2010/05/25 22:06:50, Aaron Boodman wrote: > On Tue, May 25, 2010 at 2:07 PM, ...
10 years, 7 months ago (2010-05-25 22:14:50 UTC) #10
Aaron Boodman
On Tue, May 25, 2010 at 3:14 PM, <mpcomplete@chromium.org> wrote: > On 2010/05/25 22:06:50, Aaron ...
10 years, 7 months ago (2010-05-25 22:32:26 UTC) #11
Peter Kasting
http://codereview.chromium.org/2078021/diff/1/19 File chrome/browser/search_engines/template_url_model.cc (right): http://codereview.chromium.org/2078021/diff/1/19#newcode1087 chrome/browser/search_engines/template_url_model.cc:1087: UTF8ToWide(extension->id()) + L"/?q={searchTerms}", 0, 0); On 2010/05/25 21:07:13, Matt ...
10 years, 7 months ago (2010-05-26 01:10:41 UTC) #12
Matt Perry
Tests added. Will get to Peter's comments next.
10 years, 7 months ago (2010-05-26 01:30:12 UTC) #13
Matt Perry
http://codereview.chromium.org/2078021/diff/1/19 File chrome/browser/search_engines/template_url_model.cc (right): http://codereview.chromium.org/2078021/diff/1/19#newcode1087 chrome/browser/search_engines/template_url_model.cc:1087: UTF8ToWide(extension->id()) + L"/?q={searchTerms}", 0, 0); On 2010/05/26 01:10:42, Peter ...
10 years, 7 months ago (2010-05-26 22:04:23 UTC) #14
Peter Kasting
http://codereview.chromium.org/2078021/diff/23001/24003 File chrome/browser/autocomplete/keyword_provider.cc (right): http://codereview.chromium.org/2078021/diff/23001/24003#newcode152 chrome/browser/autocomplete/keyword_provider.cc:152: matches_.insert(matches_.begin(), On 2010/05/26 22:04:25, Matt Perry wrote: > On ...
10 years, 7 months ago (2010-05-26 22:45:43 UTC) #15
Matt Perry
http://codereview.chromium.org/2078021/diff/23001/24003 File chrome/browser/autocomplete/keyword_provider.cc (right): http://codereview.chromium.org/2078021/diff/23001/24003#newcode152 chrome/browser/autocomplete/keyword_provider.cc:152: matches_.insert(matches_.begin(), On 2010/05/26 22:45:43, Peter Kasting wrote: > On ...
10 years, 7 months ago (2010-05-26 23:08:56 UTC) #16
Peter Kasting
LGTM pending implementation of the minimal_changes fix we discussed in person; you should also consider ...
10 years, 7 months ago (2010-05-26 23:19:43 UTC) #17
Aaron Boodman
LGTM
10 years, 7 months ago (2010-05-27 22:26:06 UTC) #18
Peter Kasting
http://codereview.chromium.org/2078021/diff/64001/65003 File chrome/browser/autocomplete/keyword_provider.cc (right): http://codereview.chromium.org/2078021/diff/64001/65003#newcode155 chrome/browser/autocomplete/keyword_provider.cc:155: done_ = true; We shouldn't set done_ = true ...
10 years, 7 months ago (2010-05-27 23:42:14 UTC) #19
Matt Perry
http://codereview.chromium.org/2078021/diff/64001/65003 File chrome/browser/autocomplete/keyword_provider.cc (right): http://codereview.chromium.org/2078021/diff/64001/65003#newcode155 chrome/browser/autocomplete/keyword_provider.cc:155: done_ = true; On 2010/05/27 23:42:14, Peter Kasting wrote: ...
10 years, 7 months ago (2010-05-27 23:49:29 UTC) #20
Peter Kasting
10 years, 7 months ago (2010-05-27 23:55:53 UTC) #21
LGTM

Powered by Google App Engine
This is Rietveld 408576698