Implement a simple Extension App Omnibox provider.

chrome/browser/autocomplete/extension_app_provider.cc

+// Copyright (c) 2011 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "chrome/browser/autocomplete/extension_app_provider.h"
+
+#include <algorithm>
+
+#include "base/string16.h"
+#include "base/utf_string_conversions.h"
+#include "chrome/browser/autocomplete/autocomplete_match.h"
+#include "chrome/browser/extensions/extension_service.h"
+#include "chrome/browser/profiles/profile.h"
+#include "content/common/notification_service.h"
+#include "ui/base/l10n/l10n_util.h"
+
+ExtensionAppProvider::ExtensionAppProvider(ACProviderListener* listener,
+                                           Profile* profile)
+    : AutocompleteProvider(listener, profile, "ExtensionApps") {
+  RefreshAppList();
+  RegisterForNotifications();

[mrossetti, 2011/03/31 16:44:17]

I don't know — I'm just askin': Is it possible for an app notification to come
in in between the call to 'RefreshAppList' and the registration for
notifications?

[Finnur, 2011/04/01 15:49:36]

I don't think so, but I also think that reversing the order is harmless. Done.

On 2011/03/31 16:44:17, mrossetti wrote:

+}
+
+ExtensionAppProvider::ExtensionAppProvider(ACProviderListener* listener,

[Peter Kasting, 2011/03/31 21:33:59]

Will you really be using this?  A TemplateURLModel is a very odd thing for a
test to give you...

[Finnur, 2011/04/01 15:49:36]

Done.

+                                           TemplateURLModel* model)
+    : AutocompleteProvider(listener, NULL, "ExtensionApps") {
+  RefreshAppList();
+  RegisterForNotifications();
+}
+
+void ExtensionAppProvider::Start(const AutocompleteInput& input,
+                                 bool minimal_changes) {
+  matches_.clear();
+
+  if (!input.text().empty()) {

[Peter Kasting, 2011/03/31 21:33:59]

Your changes to autocomplete.h claimed you only acted on UNKNOWN input types,
but this makes no such checks. You should at least reject INVALID input, and you
should update the other tables with your scores.

[Finnur, 2011/04/01 15:49:36]

Done.

+    std::string input_utf8 = WideToUTF8(input.text());
+    for (std::vector<ExtensionApp>::const_iterator app =
+             extension_apps_.begin();
+         app != extension_apps_.end(); ++app) {
+      // See if the input matches this extension application.
+      const std::string& name = (*app).first;

[mrossetti, 2011/03/31 16:44:17]

Nit: "(*app)." ==> "app->", here and the next line.

[Finnur, 2011/04/01 15:49:36]

Doh! Done.

On 2011/03/31 16:44:17, mrossetti wrote:

+      const std::string& url = (*app).second;
+      std::string::const_iterator name_iter =
+          std::search(name.begin(),
+                      name.end(),
+                      input_utf8.begin(),
+                      input_utf8.end(),
+                      base::CaseInsensitiveCompare<char>());
+      std::string::const_iterator url_iter =
+          std::search(url.begin(),
+                      url.end(),
+                      input_utf8.begin(),
+                      input_utf8.end(),
+                      base::CaseInsensitiveCompare<char>());
+
+      bool matches_name = name_iter != name.end();
+      bool matches_url = url_iter != url.end();

[mrossetti, 2011/03/31 16:44:17]

Nit: No need for this 'matches_url' temporary. Just use "url_iter != url.end()"
in the 'if' that follows. And 'matches_name' also probably falls into this
category.

[Finnur, 2011/04/01 15:49:36]

I think it improves readability. |matches_name| is also used down below.

On 2011/03/31 16:44:17, mrossetti wrote:

+      if (matches_name || matches_url) {
+        bool complete_match = input.text().length() == name.length();
+        // We have a match, might be a partial match.
+        // TODO(finnur): Figure out what type to return here, might want to have
+        // the extension icon/a generic icon show up in the Omnibox.
+        AutocompleteMatch match(this, 0, false, AutocompleteMatch::HISTORY_URL);

[mrossetti, 2011/03/31 16:44:17]

Yeah, I'd think we'd want an app type icon at a minimum rather than it showing
up as a history match. I'm not sure what I think about having an app-specific
icon since it might be better to have the this-is-an-app visual clue versus
this-is-TweetDeck.

[Finnur, 2011/04/01 15:49:36]

Yeah, I'm on the this-is-an-app visual clue side. I think it would be a bit
jarring visually to have the extension icon show up, but that's what the mocks
called for. For now, the globe looks visually appropriate, so I left it at that
until I can get icons from the designers (another changelist).

On 2011/03/31 16:44:17, mrossetti wrote:

+        match.fill_into_edit = input.text();

[Peter Kasting, 2011/03/31 21:33:59]

No, you want the string that someone should see when in the omnibox when
arrowing down to this choice, which is either the full name or the full URL.

[Finnur, 2011/04/01 15:49:36]

Good point.

On 2011/03/31 21:33:59, Peter Kasting wrote:

+        match.destination_url = GURL(url);
+        match.inline_autocomplete_offset = string16::npos;
+        /* Will add this to the generated_resources.grd (currently has a bunch
+           of other changes in my tree from another CL):
+          <!-- Extension App Provider strings -->
+          <message name="IDS_EXTENSION_APP_LAUNCH_PREFIX"
+                   desc="App launcher prefix for the Omnibox.">
+            Launch

[Peter Kasting, 2011/03/31 21:33:59]

You need to use "Launch $1", not just give a prefix.

[Finnur, 2011/04/01 15:49:36]

I opted to remove this string. It wasn't part of the mocks and it just
complicates things in i18n when the parameter might be on the left of the Launch
word, conceivably.

On 2011/03/31 21:33:59, Peter Kasting wrote:

+          </message>
+        */
+        string16 prefix = ASCIIToWide("Launch ");
+        match.contents = prefix + UTF8ToWide(name);
+        match.contents_class.push_back(
+            ACMatchClassification(0, ACMatchClassification::DIM));
+        if (name_iter != name.end()) {
+          size_t pos = name_iter - name.begin();
+          match.contents_class.push_back(
+              ACMatchClassification(prefix.length() + pos,
+                                    ACMatchClassification::MATCH));
+          if (pos + input.text().length() < name.length())
+            match.contents_class.push_back(
+                ACMatchClassification(prefix.length() + pos +
+                                          input.text().length(),
+                                      ACMatchClassification::DIM));
+        }
+
+        match.description = UTF8ToWide(url);
+        match.description_class.push_back(
+              ACMatchClassification(0, ACMatchClassification::DIM));
+        if (url_iter != url.end()) {
+          size_t pos = url_iter - url.begin();
+          match.description_class.push_back(
+              ACMatchClassification(pos, ACMatchClassification::MATCH));
+          if (pos + input.text().length() < url.length())
+            match.description_class.push_back(
+                ACMatchClassification(pos + input.text().length(),
+                                      ACMatchClassification::DIM));
+        }
+
+        match.relevance = CalculateRelevance(input.type(),
+                                             complete_match,
+                                             matches_name);
+        matches_.push_back(match);
+      }
+    }
+  }
+
+  done_ = true;

[Peter Kasting, 2011/03/31 21:33:59]

Nit: You don't need this.

[Finnur, 2011/04/01 15:49:36]

Done.

+}
+
+void ExtensionAppProvider::Stop() {

[Peter Kasting, 2011/03/31 21:33:59]

Nit: You don't need to override this.

[Finnur, 2011/04/01 15:49:36]

Done.

+  done_ = true;
+}
+
+ExtensionAppProvider::~ExtensionAppProvider() {
+}
+
+void ExtensionAppProvider::RefreshAppList() {
+  ExtensionService* extensions_service = profile_->GetExtensionService();

[Peter Kasting, 2011/03/31 21:33:59]

Nit: Roll this into the next statement.

[Finnur, 2011/04/01 15:49:36]

Done.

+  const ExtensionList* extensions = extensions_service->extensions();
+
+  extension_apps_.clear();

[Peter Kasting, 2011/03/31 21:33:59]

Nit: No blank lines above/below this.

[Finnur, 2011/04/01 15:49:36]

Done.

+
+  for (ExtensionList::const_iterator app = extensions->begin();
+       app != extensions->end(); ++app) {
+     if (!(*app)->is_app() || (*app)->launch_web_url().empty())

[Peter Kasting, 2011/03/31 21:33:59]

Nit: Reverse the conditional and do the push_back() conditionally.

[Finnur, 2011/04/01 15:49:36]

Done.

+       continue;
+
+     extension_apps_.push_back(std::make_pair((*app)->name(),
+                                              (*app)->launch_web_url()));

[Peter Kasting, 2011/03/31 21:33:59]

What if this is a relative URL?

[Finnur, 2011/04/01 15:49:36]

That is never supposed to happen and we'd have problems in other parts of the
code as well (or it would lead to a degraded user experience, I should say).

Besides, if it is not a full URL then clicking on the app won't work and we have
no way of fixing it up anywhere (not just here), since we don't have the base
URL stored. This is basically the only information we have about where to take
you when you click the app. If this doesn't work, we have bigger problems. :)

On 2011/03/31 21:33:59, Peter Kasting wrote:

[Peter Kasting, 2011/04/01 16:32:10]

Huh.  Maybe I misread the comments on its declaration.  If that's the case, it
seems like it'd be a good idea to make it be a GURL rather than a string (both
the code here as well as the |launch_web_url_| itself), don't you think?

[Finnur, 2011/04/01 18:10:40]

Um... Sure. I can do that in a followup changelist.

On 2011/04/01 16:32:10, Peter Kasting wrote:

+  }
+}
+
+void ExtensionAppProvider::RegisterForNotifications() {
+  registrar_.Add(this, NotificationType::EXTENSION_LOADED,
+                 NotificationService::AllSources());
+  registrar_.Add(this, NotificationType::EXTENSION_UNINSTALLED,
+                 NotificationService::AllSources());
+}
+
+
+// static
+int ExtensionAppProvider::CalculateRelevance(AutocompleteInput::Type type,

[mrossetti, 2011/03/31 16:44:17]

Driveby: I'd probably put CalculateRelevance at the end of the file after
Observe so that it's not stuck between two functions that have something to do
with notifications.

[Finnur, 2011/04/01 15:49:36]

Done.

+                                             bool complete,
+                                             bool matches_name) {
+  // TODO(finnur): Need to figure out what to return here.

[Peter Kasting, 2011/03/31 21:33:59]

You definitely shouldn't return the exact value another provider returns, or
your sort will be arbitrary (and possibly unstable).

+  return 1200;

[mrossetti, 2011/03/31 16:44:17]

Driveby: Yeah, a value based at least on 1) how far from the start of the string
the match occurs, and 2) how much of the string matches would probably be a good
start. And if it's a URL match which starts at the beginning of the string it is
a legitimate candidate for the primary in-the-omnibox candidate.

At some point I'm going to figure out how to allow really good matches that
don't match from the beginning of the string to be the top in-the-omnibox
autocomplete suggestion, but for now we can only do that for URL matches which
start at the beginning of the string.

[Finnur, 2011/04/01 15:49:36]

I opted for a simple sliding scale of 850 to 1450 based on how many letters
you've typed. I'm hesitant to associate the relevance with the location of the
match found, since I'm not sure that partial search at the end of the string is
better than a partial match at the beginning of the string. But we can always
tweak this later.

On 2011/03/31 16:44:17, mrossetti wrote:

[Peter Kasting, 2011/04/01 16:32:10]

Be careful.  1450 is above history exact match/inline autocompletion.  That may
not be what you want.  (For example, if I type "moma" in my address bar, the
navigational entry for the intranet site scores 1400, so an app that began with
"moma..." could prevent me from navigating there at all).

You probably want to score based primarily on typed count, more than length of
matched string.

[Finnur, 2011/04/01 18:10:40]

My thinking was that the likelyhood of you installing an app with a conflicting
name of something you want to navigate to  is pretty low. I tried lower values
and they worked ok, too. If I lower it to say max 1200, would you accept that as
a patch?

On 2011/04/01 16:32:10, Peter Kasting wrote:

[Finnur, 2011/04/01 20:29:10]

Yes? No? We are nearing the deadline for M12 features enabled on the branch and
we can spend next week tweaking the scoring as necessary...

On 2011/04/01 18:10:40, Finnur wrote:

+}

[Finnur, 2011/03/31 22:25:20]

Thank you both for your comments...

It is late, and I'll address both your comments tomorrow, but I want to get the
discussion going on this point early...

I thought about something similar to what Mike proposed, but these numbers don't
mean anything to me (except that higher values lead to higher rankings). Someone
said 1200 was way too high as a maximum value but I don't know what is a good
lower bound and what is a good upper bound... 1-1000? 500-750? And if it is a
sliding scale, won't I have problems if it hits the exact value of some other
provider (unstable sort and all), as Peter points out?

I also don't know what types I should act on... only UNKNOWN? Combination of
some types? Something else?

+
+void ExtensionAppProvider::Observe(NotificationType type,
+                                   const NotificationSource& source,
+                                   const NotificationDetails& details) {
+  switch (type.value) {
+    case NotificationType::EXTENSION_LOADED:
+    case NotificationType::EXTENSION_UNINSTALLED:
+      RefreshAppList();
+      break;
+  }
+}