It should use OpenSearch ( http://en.wikipedia.org/wiki/OpenSearch )
Possible implementation:
1. it show suggestion for current keyword (like in firefox)
2. it show search result for current keyword
3. just a placeholder. when click on them, browser start with search result

Since there is a lot of search engines(eng wikipedia, rus wikipedia, eng google, rus google ......), UI for install/enable/disable search provider should be part of GnomeShell.

Created attachment 165666 [details] [review]
Add OpenSearchProviders

It slightly different from mockup.
Every open search provider is just a "search result" (not a section header), so they accessible from keyboard.
descriptions of other search provider can be found on https://addons.mozilla.org/en-US/firefox/browse/type:4/cat:all?sort=weeklydownloads .
(g_app_info_launch_default_for_uri require gvfs, so I add it to gnome-shell.modules)

Review of attachment 165666 [details] [review]:

I like the idea of using OpenSearch to define the search engines. I don't think making this a search provider will give us the right user interface - especially in the new mockups, where search results are horizontal rows of icons, it doesn't make sense to have search results which are actually "Search Google" - I think that will be confusing to the users. The buttons as mocked up more accurately represent the difference between "Search" and "Search result"

::: data/org.gnome.shell.gschema.xml.in
@@ +38,3 @@
       </_description>
     </key>
+    <key name="enabled-open-search-providers" type="as">

Why does this need to be configurable? How do you see the configuration looking in the user interface?

::: data/search_providers/googlecom-in-english.xml
@@ +1,3 @@
+<SearchPlugin xmlns="http://www.mozilla.org/2006/browser/search/" xmlns:os="http://a9.com/-/spec/opensearch/1.1/">
+<os:ShortName>Google.com (in English)</os:ShortName>
+<os:Description>Google.com that doesn't default to local Google site when outside US, without restricting search to the English language</os:Description>

This seems to be some variant of the google search, rather than what we want; the search provider with firefox, is just:

<ShortName>Google</ShortName>
<Description>Google Search</Description>

::: data/search_providers/wikipedia-eng.xml
@@ +1,2 @@
+<SearchPlugin xmlns="http://www.mozilla.org/2006/browser/search/" xmlns:os="http://a9.com/-/spec/opensearch/1.1/">
+  <os:ShortName>Wikipedia (Eng)</os:ShortName>

I don't think we want to offer a bunch of options, so we need to have a "Wikipedia" that is the right thing for the user (or at least normally so). So, we probably need to do something using the Language element and language parameters in opensearch. I'm not sure how the name is suppposed to be translated - maybe xml:lang ?

::: js/ui/search.js
@@ +265,3 @@
+
+    activateResult: function(id) {
+        let term = '';

Bit strange of a name - maybe searchTerms to match the parameter name?

@@ +267,3 @@
+        let term = '';
+        for (let i = 0; i < this._terms.length; i++)
+            term += (i ? ' ' : '') + this._terms[i];

Not sure if it's standard, but we use Array.join elsewhere in the shell, so you should be able to just do:

 term = terms.join(' ');

::: src/shell-global.c
@@ +850,3 @@
+ * @error: location to store GError
+ *
+ * Return value: (transfer none): A new #ClutterActor with the icon of provider

I don't think the the image is different from the other returns. This function returns a name, an URL, and an image. Having the image as the return value and the other returns as out parameters implies that the image is special. I'd use 3 out parameters, and then a boolean return that is TRUE if and only if *error is NULL. (A standard pattern.)

@@ +857,3 @@
+                                    char **name,
+                                    char **url,
+                                    GError **error)

Parameters should be lined up.

@@ +859,3 @@
+                                    GError **error)
+{
+  xmlDocPtr doc = xmlParseMemory (data, strlen(data));

It looks to me like you are relying on librsvg to pull in libxml as a dependency. It would be better to list it directly in the configure.ac

@@ +863,3 @@
+  ClutterActor *icon = NULL;
+
+  g_assert (name && url);

This doesn't seem to add anything - if name is NULL, the next line will segfault in a clear fashion. In general, I think g_assert is best used to check things that must be true, but aren't obviously true.

In libraries, we are generally very defensive and we want things to fail with very clear error messages, so we use g_return_if_fail() at the beginning of the function to check parameterss for valid values, but it isn't necessary here.

(In libraries, we also always support passing in NULL for out value parameters.)

@@ +866,3 @@
+  *name = NULL;
+  *url = NULL;
+  g_return_val_if_fail (doc, NULL);

Don't use g_return_if_fail() for things that aren't under the control of the calling code - it's meant for programming errors. This should be something like:

 if (doc == NULL) 
   {
      g_set_error(....);
      return NULL;
   }

@@ +869,3 @@
+
+  root = xmlDocGetRootElement (doc);
+  if (root && root->name && xmlStrcmp (root->name, (const xmlChar *)"SearchPlugin") == 0)

I think the only real value of using libxml over GMarkup (except for a slightly more convenient API) is its namespace support. So, we should actually check namespaces, instead of just assuming that SearchPlugin is os:SearchPlugin

@@ +871,3 @@
+  if (root && root->name && xmlStrcmp (root->name, (const xmlChar *)"SearchPlugin") == 0)
+    {
+      xmlNode *j;

i,j,k should be reserved for integer indices. It can be hard to come up with names when you have nested iteration ... I'd probably use 'child' and 'subchild' here.

@@ +888,3 @@
+                if (!val)
+                    continue;
+                if (strncmp ((char*)val, pre, strlen (pre)) == 0)

g_str_has_prefix() is more readable.

@@ +893,3 @@
+                    guchar *data = g_base64_decode ((const char*)(val + strlen (pre)), &len);
+                    GdkPixbufLoader *loader = gdk_pixbuf_loader_new ();
+                    if (gdk_pixbuf_loader_write (loader, data, len, error))

Once error has been set, you can't just keep on using it - overwriting a set GError will cause the g_error() code to warn. If you really wanted to use the 'error' passed in, then you should propagate the error out to the caller.

I think we generally want parsing of these files to be strict - if something is wrong in them, the search provider shouldn't be loaded, and a clear error message should be logged.

@@ +908,3 @@
+                xmlNode *i;
+                xmlChar *val;
+                xmlChar *type = xmlGetProp(j, (xmlChar*)"type");

This initialization doesn't refer to the whole block, but is closely tied to the following lines, so I'd do:

 xmlChar *val;
 xmlChar *type;

 type = xmlGetProp(...);
 if (!type)
    continue;

@@ +915,3 @@
+
+                if (b)
+                  continue;

tests on strcmp() returns should always be written as b != 0, b == 0, to make clear it's not a boolean result. This ceck should be grouped with the previous lines not the following lines.

@@ +916,3 @@
+                if (b)
+                  continue;
+                val = xmlGetProp(j, (const xmlChar*)"template");

this isn't more of a generic 'val' than 'type' is, so, it's confusing to have one 'type' and one 'val' - the general pattern you seem to be using here is to call the variable the same as the attribute, so 'template'

@@ +925,3 @@
+                  {
+                    xmlChar *name;
+                    xmlChar *value;

Can't have two variables in the same function, 'val' and 'value'

@@ +928,3 @@
+                    if (!i->name)
+                      continue;
+                    if (xmlStrcmp (i->name, (const xmlChar *)"Param") != 0)

I don't see this element anywhere in the opensearch specification. Do you have a pointer to where this is defined?

@@ +933,3 @@
+                    value = xmlGetProp(i, (const xmlChar*)"value");
+                    if (name && value)
+                      param = g_strconcat (param ? param : "", param ? "&" : "", name, "=", value, NULL);

Does this need URL escaping?

@@ +937,3 @@
+                    xmlFree (value);
+                  }
+                  if (param)

Identation problem.

::: src/shell-global.h
@@ +73,3 @@
 void shell_global_breakpoint (ShellGlobal *global);
 
+ClutterActor* shell_global_parse_search_provider (ShellGlobal *global, const char  *data, char **name, char **url, GError **error);

Parameters should be broken onto multiple lines

Created attachment 168530 [details] [review]
add annotation for g_get_language_names

Created attachment 168531 [details] [review]
add ability to search in web from dash

> Why does this need to be configurable?
For disabling search engines. Otherwise Disabling search engine will require root permission (remove file in /usr/share).

> How do you see the configuration looking
> in the user interface?
We already have gnome-shell-clock-preferences. Lets extend it for whole Gnome-Shell. 

> I don't think we want to offer a bunch of options, so we need to have a
> "Wikipedia" that is the right thing for the user (or at least normally so).
> we probably need to do something using the Language element and language
> parameters in opensearch.
done

> So, we should actually check
> namespaces, instead of just assuming that SearchPlugin is os:SearchPlugin
Namespace can be empty.

> I don't see this element anywhere in the opensearch specification. Do you have
> a pointer to where this is defined?

http://www.docshare.com/doc/196079/OpenSearch-Cheat-Sheet
https://developer.mozilla.org/en/creating_opensearch_plugins_for_firefox

Review of attachment 168530 [details] [review]:

Really a scanner bug (it should get that prototype automatically) but annotation is harmless when that bug is fixed. Good to commit.

> > So, we should actually check
> > namespaces, instead of just assuming that SearchPlugin is os:SearchPlugin
> Namespace can be empty.

In:

 <?xml version="1.0" encoding="UTF-8"?>
 <OpenSearchDescription xmlns="http://a9.com/-/spec/opensearch/1.1/">
   <ShortName>Web Search</ShortName>
   [....]
 </OpenSearchDescription>

Every element has a namespace.

 <?xml version="1.0" encoding="UTF-8"?>
 <OpenSearchDescription>
   <ShortName>Web Search</ShortName>
   [....]
 </OpenSearchDescription>

Is not a valid OpenSearch document.

Review of attachment 168531 [details] [review]:

Generally looking good. Still a good number of comments.

::: data/org.gnome.shell.gschema.xml.in
@@ +38,3 @@
       </_description>
     </key>
+    <key name="enabled-open-search-providers" type="as">

This should be reversed (disabled-open-search-provders) Otherwise if the user has ever set this, they won't see new search providers installed on the system until they go to the dialog.

::: data/search_providers/google.xml
@@ +1,1 @@
+<SearchPlugin xmlns="http://www.mozilla.org/2006/browser/search/">

This is not OpenSearch, it's a predecessor spec to OpenSearch. Seems better to implement OpenSearch and only OpenSearch

@@ +4,3 @@
+<InputEncoding>UTF-8</InputEncoding>
+<Image width="16" height="16">data:image/png;base64,AAABAAEAEBAAAAEAGABoAwAAFgAAACgAAAAQAAAAIAAAAAEAGAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAADs9Pt8xetPtu9FsfFNtu%2BTzvb2%2B%2Fne4dFJeBw0egA%2FfAJAfAA8ewBBegAAAAD%2B%2FPtft98Mp%2BwWsfAVsvEbs%2FQeqvF8xO7%2F%2F%2F63yqkxdgM7gwE%2FggM%2BfQA%2BegBDeQDe7PIbotgQufcMufEPtfIPsvAbs%2FQvq%2Bfz%2Bf%2F%2B%2B%2FZKhR05hgBBhQI8hgBAgAI9ewD0%2B%2Fg3pswAtO8Cxf4Kw%2FsJvvYAqupKsNv%2B%2Fv7%2F%2FP5VkSU0iQA7jQA9hgBDgQU%2BfQH%2F%2Ff%2FQ6fM4sM4KsN8AteMCruIqqdbZ7PH8%2Fv%2Fg6Nc%2Fhg05kAA8jAM9iQI%2BhQA%2BgQDQu6b97uv%2F%2F%2F7V8Pqw3eiWz97q8%2Ff%2F%2F%2F%2F7%2FPptpkkqjQE4kwA7kAA5iwI8iAA8hQCOSSKdXjiyflbAkG7u2s%2F%2B%2F%2F39%2F%2F7r8utrqEYtjQE8lgA7kwA7kwA9jwA9igA9hACiWSekVRyeSgiYSBHx6N%2F%2B%2Fv7k7OFRmiYtlAA5lwI7lwI4lAA7kgI9jwE9iwI4iQCoVhWcTxCmb0K%2BooT8%2Fv%2F7%2F%2F%2FJ2r8fdwI1mwA3mQA3mgA8lAE8lAE4jwA9iwE%2BhwGfXifWvqz%2B%2Ff%2F58u%2Fev6Dt4tr%2B%2F%2F2ZuIUsggA7mgM6mAM3lgA5lgA6kQE%2FkwBChwHt4dv%2F%2F%2F728ei1bCi7VAC5XQ7kz7n%2F%2F%2F6bsZkgcB03lQA9lgM7kwA2iQktZToPK4r9%2F%2F%2F9%2F%2F%2FSqYK5UwDKZAS9WALIkFn%2B%2F%2F3%2F%2BP8oKccGGcIRJrERILYFEMwAAuEAAdX%2F%2Ff7%2F%2FP%2B%2BfDvGXQLIZgLEWgLOjlf7%2F%2F%2F%2F%2F%2F9QU90EAPQAAf8DAP0AAfMAAOUDAtr%2F%2F%2F%2F7%2B%2Fu2bCTIYwDPZgDBWQDSr4P%2F%2Fv%2F%2F%2FP5GRuABAPkAA%2FwBAfkDAPAAAesAAN%2F%2F%2B%2Fz%2F%2F%2F64g1C5VwDMYwK8Yg7y5tz8%2Fv%2FV1PYKDOcAAP0DAf4AAf0AAfYEAOwAAuAAAAD%2F%2FPvi28ymXyChTATRrIb8%2F%2F3v8fk6P8MAAdUCAvoAAP0CAP0AAfYAAO4AAACAAQAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAACAAQAA</Image>
+<Url type="application/x-suggestions+json" method="GET" template="http://suggestqueries.google.com/complete/search?output=firefox&amp;client=firefox&amp;hl={moz:locale}&amp;q={searchTerms}"/>

client is not firefox.

@@ +6,3 @@
+<Url type="application/x-suggestions+json" method="GET" template="http://suggestqueries.google.com/complete/search?output=firefox&amp;client=firefox&amp;hl={moz:locale}&amp;q={searchTerms}"/>
+<Url type="text/html" method="GET" template="http://www.google.com/search">
+  <Param name="q" value="{searchTerms}"/>

<Param/> is not part of OpenSearch, see https://developer.mozilla.org/Talk:en/Creating_OpenSearch_plugins_for_Firefox. There's a proposed extension for <Parameter/> but I don't think we need it - we can just put the parameters in the template=""

::: data/search_providers/wikipedia.xml
@@ +10,3 @@
+<Url type="text/html" method="GET" template="http://{language}.wikipedia.org/wiki/Special:Search">
+  <Param name="search" value="{searchTerms}"/>
+  <Param name="sourceid" value="Mozilla-search"/>

We shoudn't be sending queries with sourceid=Mozilla-search

@@ +13,3 @@
+</Url>
+<Language>ru</Language>
+<Language>en</Language>

I don't want to deal with patches from everyone to add their language. So can you make this 

<!-- The criterion for being below is being listed with more than 100,000 articles 
     on http://meta.wikimedia.org/wiki/List_of_Wikipedias -->
[ all the current qualifiying wikipedias, sorted alphabetically ]


You should be able to extract that list fairly easily by cutting and pasting the section of the page into a text file and then processing with perl/awk/sort/etc. or there are only 35, so it's not too bad to do manually if you want to do it that way.

::: js/ui/dash.js
@@ +422,3 @@
+
+        this._placeHolder = new St.Bin({ visible: false });
+        this.actor.add(this._placeHolder, { expand:true, x_fill: false, y_fill: true });

'placeholder' is one word, so this._placeholder. You need to add a comment here explaining why you need a placeholder.

@@ +434,3 @@
+    },
+
+    createOpenSearchProviderButton: function(provider) {

Private method so should be _createOpenSearchProviderButton

@@ +438,3 @@
+                                             reactive: true,
+                                             visible: false });
+        providerBox.connect('button-press-event', Lang.bind(this, function() {

Seems like you should use St.Clickable

@@ +511,3 @@
+        for (let i = 0; i < list.length; i++)
+            if (list[i].visible)
+                height += list[i].height;

it's bad practice to synchronously query size (this forces an out-of-band allocation of the entire stage)

Maybe you can use St.OverflowBox here. 

But I think it's probably better to just show all the providers. We'll be redoing the search layout with the overview layout changes. So we shouldn't get fancy now.

::: js/ui/search.js
@@ +231,3 @@
+        this._container = new St.Group({ visible: false });
+        /* source of ClutterClone should realized */
+        Main.uiGroup.add_actor(this._container);

Clones are best used for something like the shell magnifier where you actually need the same actor content in multiple places. I don't think they make sense for just sharing an image. The best thing would be to use a CoglTexture for the icon rather than a ClutterTexture. However, we can't use CoglTexture from JS. (It's not a GObject)

My workaround suggestion: return the data:// URI as a string from the function and make st_texture_cache_load_uri handle data:// URIs. That will also fix something else:
st_texture_cache_convert() doesn't make sense as a method on StTextureCache.

It's a little inefficient to have data:// URIs in a hash table - strcmp() is slow on a very long URI. But I don't think it will hurt much in practice.

@@ +250,3 @@
+    },
+
+    _isProviderSupportLocalLanguage: function(provider) {

gramatically strange. Suggest '_providerSupportsLocalLanguage'. Or probably better '_providerSupportsLocaleLanguage' (LANG sets your 'locale'.)

@@ +257,3 @@
+                for (let k = 0; k < provider.langs.length; k++) {
+                    if (langs[i] == provider.langs[k])
+                        lang = langs[i];

Could just 'return true' here - but see below

@@ +278,3 @@
+                for (let k = 0; k < this._providers[id].langs.length; k++) {
+                    if (langs[i] == this._providers[id].langs[k])
+                        lang = langs[i];

I don't like having this looping/matching logic in two places; what if you just substituted {language} in addProvider?

@@ +295,3 @@
+        let dis = new Gio.DataInputStream({ 'base-stream': is });
+        let source = '';
+        dis.read_line_async(GLib.PRIORITY_LOW, null, Lang.bind(this, function onReadLineComplete(obj, res) {

Avoiding doing blocking IO in the main thread is good practice. But this is a lot of messy complexity for reading a few bytes. It looks like using Gio.File.load_contents_async() will work fine.

@@ +305,3 @@
+                                icon: icon,
+                                langs: langs };
+                if (this._isProviderSupportLocalLanguage(provider)) {

Hmm, if your locale is not supported by Wikipedia, it's probably better to to provide the English Wikipedia rather than no Wikipedia.

::: src/shell-global.c
@@ +921,3 @@
+
+  for (i = 0; pre[i]; i++)
+      if (g_str_has_prefix ((char*)val, pre[i]))

Use {} in this case for clarity - this doesn't count as a single-line body of the for loop.

@@ +925,3 @@
+          gsize len;
+          guchar *data = NULL;
+          gchar  *unescaped;

Stray space

@@ +927,3 @@
+          gchar  *unescaped;
+
+          unescaped = g_uri_unescape_string ((const char*)(val + strlen (pre[i])), NULL);

'const char *'

@@ +940,3 @@
+              loader = gdk_pixbuf_loader_new ();
+              if (gdk_pixbuf_loader_write (loader, data, len, NULL))
+                if (gdk_pixbuf_loader_close (loader, NULL))

Better to use && then nested ifs. (If you do use nested ifs, then the outer if should have {} for clarity.)

@@ +953,3 @@
+ * shell_global_parse_search_provider:
+ * @global: A #ShellGlobal
+ * @name: (out): location to store a display name

Best to include all parameters, you are missing @data

@@ +955,3 @@
+ * @name: (out): location to store a display name
+ * @url: (out): location to store template of url
+ * @langs: (out) (transfer full)(element-type utf8): list of supported languages

') ('

@@ +956,3 @@
+ * @url: (out): location to store template of url
+ * @langs: (out) (transfer full)(element-type utf8): list of supported languages
+ * @actor: (out) (transfer none): location to store icon

Parameter name should be what it is, what not type it is. So 'icon' or 'icon_actor' not 'actor'

@@ +986,3 @@
+
+  root = xmlDocGetRootElement (doc);
+  if (root && root->name &&

Can root really be null here? Every valid XML document has a root.

@@ +987,3 @@
+  root = xmlDocGetRootElement (doc);
+  if (root && root->name &&
+      (xmlStrcmp (root->name, (const xmlChar *)"SearchPlugin") == 0 ||

I don't think we need to support the Mozilla search plugin stuff - it's easy enough to convert a mozilla search plugin to opensearch.

@@ +1013,3 @@
+                xmlChar *val = xmlNodeListGetString(doc, child->xmlChildrenNode, 1);
+                if (val)
+                  icon = decode_image (val);

You should provide a specific error if icon parsing failed.

@@ +1034,3 @@
+                xmlFree (type);
+
+                template = xmlGetProp(child, (const xmlChar*)"template");

'xmlChar *' - lots more, please check.

@@ +1054,3 @@
+                        gchar *ename = g_uri_escape_string ((gchar*)name, "{}", FALSE);
+                        gchar *evalue = g_uri_escape_string ((gchar*)value, "{}", FALSE);
+                        param = g_strconcat (param ? param : "", param ? "&" : "", ename, "=", evalue, NULL);

Would be better to use a GString for this, but as noted before I don't think we need to handle <Param/>

@@ +1088,3 @@
+
+  *url = NULL;
+  *name = NULL;

You don't handle cleaning up langs the same way

@@ +1090,3 @@
+  *name = NULL;
+
+  g_set_error (error, G_IO_ERROR, G_IO_ERROR_FAILED, "Malformed xml");

This will confuse the person trying to debug what is going wrong. Minimum is a message like "Couldn't parse search provider as OpenSearch document with icon, name, and URL." but I think it's worth trying to provide specific messages that let people track down more closely what is going wrong.

if (icon == NULL && *error == NULL)
  g_set_error (error, "...")

[...]

if (*error == NULL) 
  {
    *actor = icon;
    return TRUE;
  }
else
  {
    /* cleanup */
    return FALSE;
  }

Created attachment 170760 [details] [review]
add ability to search in web from dash

> Can root really be null here?
yes

Review of attachment 170760 [details] [review]:

Looking yet better, still various comments.

::: data/org.gnome.shell.gschema.xml.in
@@ +40,3 @@
+    <key name="disabled-open-search-providers" type="as">
+      <default>[]</default>
+      <_summary>disabled open search providers</_summary>

OpenSearch is a name, so should be correctly spelled as 'OpenSearch' in the doc strings

::: data/search_providers/wikipedia.xml
@@ +5,3 @@
+<Image width="16" height="16">data:image/x-icon;base64,AAABAAEAEBAQAAEABAAoAQAAFgAAACgAAAAQAAAAIAAAAAEABAAAAAAAAAAAAAAAAAAAAAAAEAAAAAAAAAAEAgQAhIOEAMjHyABIR0gA6ejpAGlqaQCpqKkAKCgoAPz9%2FAAZGBkAmJiYANjZ2ABXWFcAent6ALm6uQA8OjwAiIiIiIiIiIiIiI4oiL6IiIiIgzuIV4iIiIhndo53KIiIiB%2FWvXoYiIiIfEZfWBSIiIEGi%2FfoqoiIgzuL84i9iIjpGIoMiEHoiMkos3FojmiLlUipYliEWIF%2BiDe0GoRa7D6GPbjcu1yIiIiIiIiIiIiIiIiIiIiIiIiIiIiIiIgAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA</Image>
+<Url type="text/html" method="GET" template="http://{language}.wikipedia.org/wiki/Special:Search?search={searchTerms}"/>
+<Language>ar</Language>

You need an (XML) comment here explaining where this list comes from. (I had a suggested comment in my last set of review comments)

@@ +40,3 @@
+<Language>war</Language>
+<Language>zh</Language>
+<SearchForm>http://en.wikipedia.org/wiki/Special:Search</SearchForm>

SearchForm isn't an OpenSearch element

::: js/ui/dash.js
@@ +421,3 @@
             this.createProviderMeta(this._providers[i]);
+
+        /*placeholder take place between search result and search buttons*/

Generally we use // comments. But this comment doesn't tell me anything.

 Is it to add padding between the search result and the search buttons
 Is it to align the search buttons to the bottom?

@@ +432,3 @@
+            for (let i = 0; i < this._openSearchProviders.length; i++)
+                this._createOpenSearchProviderButton(this._openSearchProviders[i]);
+        }));

You seem to be counting on the fact openSearchSystem does asynchronous IO and will emit changed at some later point. I don't like relying on this (this breaks encapsulation between the different JS files). It's better to have an _updateOpenSearchProviderButtons() and to call that initially and then on ::changed.

@@ +532,3 @@
+        for (let i = 0; i < this._openSearchProviders.length; i++)
+            this._openSearchProviders[i].actor.show();
+        this._placeholder.show();

Is the idea of creating these not visible and making them visible later so they only show up when there is some search text?
If this is the case, don't you need to hide them again later? I don't see anything that hides the buttons.

::: js/ui/search.js
@@ +306,3 @@
+        let names = global.settings.get_strv(DISABLED_OPEN_SEARCH_PROVIDERS_KEY);
+        let file = Gio.file_new_for_path(global.datadir + '/search_providers');
+        file.enumerate_children_async(Gio.FILE_ATTRIBUTE_STANDARD_NAME, Gio.FileQueryInfoFlags.NONE, GLib.PRIORITY_LOW, null, function(obj, res) {

Hmm, again asnc IO is good be this seems like a ton of complexity for something simple. I wonder if we need

 FileUtils.list_dir_async(function(children) {
    ....
 });

OK for now, but this code should not appear two places in the shell :-). maybe add a comment:

 // This should be refactored into a FileUtils.list_dir_async() rather than cut-and-pasted

@@ +308,3 @@
+        file.enumerate_children_async(Gio.FILE_ATTRIBUTE_STANDARD_NAME, Gio.FileQueryInfoFlags.NONE, GLib.PRIORITY_LOW, null, function(obj, res) {
+            let enumerator = obj.enumerate_children_finish(res);
+            enumerator.next_files_async(100, GLib.PRIORITY_LOW, null, function onNextFileComplete(obj, res) {

Wow, I didn't know you could do this - both name a function and use it inline. But don't :-) define the function with a name separately, then use it.

@@ +311,3 @@
+                let files = obj.next_files_finish(res);
+                for (let i = 0; i < files.length; i++) {
+                    let b = true;

b?

::: src/shell-global.c
@@ +1028,3 @@
+
+  root = xmlDocGetRootElement (doc);
+  if (root && root->name && xmlStrcmp (root->name, (const xmlChar *)"OpenSearchDescription") == 0)

If this was false, your code will fail with the error

 ""search provider doesn't have icon"

Which would really be confusing. Think it's best to set a specific error, free the doc, and return FALSE.

::: src/st/st-texture-cache.c
@@ +1085,3 @@
+decode_image (const char *val)
+{
+  gint i;

Don't use gint. (We're not consistent about this, in our code, but in general, whenever you are writing new code for gnome-shell, don't use gint, gdouble, gchar. These are simply typedefs to int, double, and char, and are confusing rather than helpful.)

@@ +1101,3 @@
+          gchar *unescaped;
+
+          unescaped = g_uri_unescape_string ((val + strlen (pre[i])), NULL);

Excess parens

@@ +1110,3 @@
+          if (data)
+            {
+              GdkPixbufLoader *loader = gdk_pixbuf_loader_new ();

Suggest using new_with_mime_type() here with the appropriate mime types. So something like

struct {
   const char *prefix;
   const char *mime_type;
} formats[] = {
  { "data:image/x-icon;base64,", "image/x-icon" }
};

(and use G_N_ELEMENTS())

@@ +1114,3 @@
+              loader = gdk_pixbuf_loader_new ();
+              if (gdk_pixbuf_loader_write (loader, data, len, NULL) &&
+                  gdk_pixbuf_loader_close (loader, NULL))

I'd rather you passed a GError * in to this function and to the GdkPixbuf functions in so that the user got a maximally helpful error message.

@@ +1130,3 @@
+ */
+ClutterActor *
+st_texture_cache_load_data_uri (StTextureCache *cache,

Is there a reason you can't just make data: URI's work with the current st_texture_cache_load_uri_* functions?

::: tools/build/gnome-shell.modules
@@ +24,3 @@
   </autotools>
 
+  <autotools id="gvfs">

Commit message should mention that you are adding this module and why

Created attachment 171027 [details] [review]
add fileUtils.listDirAsync

Request information for a files from the directory asynchronously.


> both name a function and use it inline.
This is standart ECMAScript feature.

> But don't :-)
Why? In my opinion, code is less readable if not use this feature)

Created attachment 171028 [details] [review]
use fileUtils.listDirAsync in CommandCompleter

Created attachment 171029 [details] [review]
[gnome-shell.modules] add gvfs

g_app_info_launch_default_for_uri require gvfs

Created attachment 171030 [details] [review]
add ability to search in web from dash

Review of attachment 171027 [details] [review]:

Looks mostly fine, but the inline named function needs to go. I apologize for not responding to your earlier comment earlier.

Fine to commit if you fix the two things below.

(There's a bit of a conceptual problem with this functionality in that it will throw an uncaught exception if an error occurs from readdir() or closedir(), and our code has no way of catching that, but I don't think there's anything meaningful we could do in most cases anyways.)

::: js/Makefile.am
@@ +21,3 @@
 	ui/environment.js	\
 	ui/extensionSystem.js	\
+	ui/fileUtils.js		\

Should be in misc/ not ui/

::: js/ui/fileUtils.js
@@ +8,3 @@
+                                  GLib.PRIORITY_LOW, null, function (obj, res) {
+        let enumerator = obj.enumerate_children_finish(res);
+        enumerator.next_files_async(100, GLib.PRIORITY_LOW, null, function onNextFileComplete(obj, res) {

This is is very similar to something like:

 if (x > 5 && foo(x) == 3 && (doubleX = 2 * x) == 15) {
    bar(doubleX);
 }

Which also wouldn't be OK for our style. You are assigning a value to the "name" onNextFileComplete and using that value in the same place. This is not a readable way to write code. If I'm wondering what onNextFileComplete() is I have to look through the entirety of every line to find it.

Just write:

        function onNextFileComplete(obj, res) {
            [...];

        }
        enumerator.next_file_async(100, GLib.PRIORITY_LOW, null, onNextFileComplete);


Your version does have certain advantages - it's more consistent with how we would write it if the function wasn't "recursive", but to me that's a less important factor than not defining names in the middle of a line.

Attempting to try this I get:
JS ERROR: !!!   Exception was: Error: No JS module 'fileUtils' found in search path

Created attachment 171491 [details] [review]
use fileUtils.listDirAsync in CommandCompleter

Created attachment 171492 [details] [review]
add ability to search in web from dash

Our most recent mockup for this is:
http://git.gnome.org/browse/gnome-shell-design/plain/mockups/static/search-results.png

Of course we still have search in the dash so we can probably just orient the buttons vertically instead.  The appearance can be similar.  

Comments after some quick testing:

We'll need to add prelighting on hover.

I think the buttons can come closer to the top when there are no results.  It is a bit of a pain to have to move all the way to the bottom I think.

I don't really like the use of icons here.

Try to match the mockups as much as possible visually.

Clicking the button should bring you to the web browser - just like if you clicked an application in search.

Typing something that has "No matching results." and then hitting return doesn't seem to do anything.  I think we should probably remove the "No matching" text and make the google button default at that point.  Another reason to move the buttons up top.

Perhaps we should always show the buttons at the end, when there is room for them.

Created attachment 171699 [details] [review]
add ability to search in web from dash

> Try to match the mockups as much as possible visually.
It require linear-gradient(bug 624383). I will write patch for it.

Created attachment 171700 [details] [review]
Clicking the search button run the web browser with search results 

> Clicking the button should bring you to the web browser - just like if you
> clicked an application in search.
If browser already opened, relaunch it with url   will not open new window.
It will be a new tab (even it located in other workspace).
Workaround to impelement this (chrome/firefox).

(require annotation fix for g_app_info_get_default_for_uri_scheme)

Created attachment 171701 [details] [review]
introspection: add annotation for g_app_info_get_default_for_type/g_app_info_get_default_for_uri_scheme 

(need only for workaround)

Created attachment 171702 [details] [review]
gio-2.0.c: add annotation for g_app_info_get_default_for_type/g_app_info_get_default_for_uri_scheme 

(need only for workaround)

Created attachment 176805 [details] [review]
add ability to search in web from dash

1. merge with HEAD
2. I disabled drawing border for search buttons, because Border color incorrect work with transparent background. 
3. work without gvfs (hack from messageTray.js)

Created attachment 176806 [details] [review]
Hack: start browser always on current workspace

(still depend on 2 patch for gobject-introspection and Glib)
I think, It should not block previous patch.

Review of attachment 171701 [details] [review]:

Looks good to me

Review of attachment 171702 [details] [review]:

Looks fine to me (gio-2.0.c is supposed to be updated by calling misc/extract-gio-sources.sh; I'm not sure if you did this one that way, but I can't imagine the result would practically be any different from adding them manually.)

Comment on attachment 171702 [details] [review]
gio-2.0.c: add annotation for g_app_info_get_default_for_type/g_app_info_get_default_for_uri_scheme 

already fixed in TRUNK

Created attachment 177942 [details] [review]
add ability to search in web from dash

merge with HEAD

(In reply to comment #29)
> Created an attachment (id=177942) [details] [review]
> add ability to search in web from dash

You should update the commit message as well - you probably want something like s/dash/search view/ (the dash is the sidebar on the left).

Review of attachment 176806 [details] [review]:

This basically didn't work for me ... what I'd see typically was that I'd get a new empty browser window focused but the search would open in my old browser window. Don't think it's even right. I think we should assume that whatever happens when you launch the browser with the URI on the command line in terms of opening in a new window or opening in a new tab in an existing window is right and we shouldn't try to do something different.

There does seem to be a timestamp problem - without this patch it seems that focus-stealing-prevention always kicks in, but that, I think, is a separate issue.

Created attachment 177956 [details] [review]
add ability to search in web from search view

> You should update the commit message as well
done

Review of attachment 177956 [details] [review]:

Generally looks very close. Various minor comments below. I see one bug in testing - when typing a search, after there are no longer any matching search results the selected open search provider bounces between the different open search providers as I type each letter.

::: js/ui/search.js
@@ +1,3 @@
 /* -*- mode: js2; js2-basic-offset: 4; indent-tabs-mode: nil -*- */
 
+const Clutter = imports.gi.Clutter;

Not used

@@ +3,3 @@
+const Clutter = imports.gi.Clutter;
+const Gio = imports.gi.Gio;
+const GLib = imports.gi.GLib;

Not used

@@ +5,3 @@
+const GLib = imports.gi.GLib;
+const Lang = imports.lang;
+const Mainloop = imports.mainloop;

Not used

@@ +8,2 @@
 const Signals = imports.signals;
+const Shell = imports.gi.Shell;

Won't be used, see below

@@ +8,3 @@
 const Signals = imports.signals;
+const Shell = imports.gi.Shell;
+const St = imports.gi.St;

Not used

@@ +251,3 @@
+
+    _providerSupportsLocaleLanguage: function(provider) {
+        if (provider.url.match('{language}')) {

Better to pass in a regexp rather than rely on implicit conversion
Slightly more efficient (hence better style) to use .search() since you only care "if" not what matched
(https://developer.mozilla.org/en/JavaScript/Reference/Global_Objects/String/search)

@@ +265,3 @@
+            }
+            provider.lang = lang;
+            return lang != null;

The assignment to provider.lang is a weird unexpected side-effect of this function since the name sounds like a boolean test. I'd call the function something like checkSupportedProviderLanguage()

@@ +267,3 @@
+            return lang != null;
+        }
+        provider.lang = "";

Don't see a reason for this assignment

@@ +281,3 @@
+            Gio.app_info_launch_default_for_uri(url, global.create_app_launch_context());
+        } catch (e) {
+            // TODO: remove this after gnome 3 release

I'm OK with the workaround but this comment needs to be more explanatory about what is going on

::: js/ui/searchDisplay.js
@@ -247,3 @@
-        if (results.length == 0) {
-            this._statusText.set_text(_("No matching results."));
-            this._statusText.show();

IMO, this still should be shown - the search buttons aren't matching results - the are something else you can do.

@@ +330,2 @@
     selectUp: function(recursing) {
+        this._updateOpenSearchButtonState();

Don't see why this is necessary or makes sense ... we haven't done anything to selectedOpenSearchButton, why would we need to update the state?

@@ +371,3 @@
             }
         }
+        this._selectedOpenSearchButton++;

This could have left selecedOpenSearchButton invalid (>= length)

@@ +372,3 @@
         }
+        this._selectedOpenSearchButton++;
+        this._updateOpenSearchButtonState();

Making this weird - shouldn't it be done later?

Review of attachment 171491 [details] [review]:

I think this is OK 

(some argument that you might want to complete to the files you get as soon as you get them, but that could be confusing too - you know a file is there, but it's not showing up in the completions until later)

And less code is good!

Created attachment 178275 [details] [review]
add ability to search in web from search view

> I see one bug in testing
fixed

> This could have left selecedOpenSearchButton invalid (>= length)
_updateOpenSearchButtonState work fine with values < 0 and >=length

> Making this weird - shouldn't it be done later?
No.
I rewrite this lines, to increase readability

Review of attachment 178275 [details] [review]:

Looks good to me now. Thanks for all the work on this!