Closed Bug 1267810 Opened 8 years ago Closed 8 years ago

[Omnibox] Complete the implementation of chrome.omnibox

Categories

(WebExtensions :: Frontend, defect, P2)

defect

Tracking

(firefox52 fixed)

RESOLVED FIXED
mozilla52
Tracking Status
firefox52 --- fixed

People

(Reporter: mattw, Assigned: mattw, NeedInfo)

References

(Blocks 2 open bugs)

Details

(Keywords: dev-doc-complete, Whiteboard: [omnibox]triaged)

Attachments

(4 files, 2 obsolete files)

This bug is meant to track the implementation and testing of chrome.omnibox.onInputStarted.

Chrome docs: https://developer.chrome.com/extensions/omnibox#event-onInputStarted
Blocks: 1267811
Blocks: 1267813
Blocks: 1267814
Whiteboard: triaged → [omnibox]triaged
OS: Android → Unspecified
Assignee: nobody → mwein
Comment on attachment 8795085 [details]
Bug 1267810 - Use the new module to implement the omnibox api.

https://reviewboard.mozilla.org/r/81256/#review80214

I'm reluctant to review this without seeing a design document for how you intend to implement the whole feature.
Also, are we going to have a bikeshed over whether to call this omnibox or something else or have multiple names?
Attachment #8795085 - Flags: review?(aswan)
Comment on attachment 8795084 [details]
Bug 1267810 - Add a module for registering keywords and handling keyword input sessions.

Clearing reviews since this requires some changes and UI additions. 

With "test" as the registered keyword, we should expect the following behavior:
tes
test
test_ -> search icon is replaced by extension icon and "test" is replaced by the extension title
test_f -> onInputStarted is fired
test_
test
test_ -> search icon is replaced by extension icon and "test" is replaced by the extension title
test__ -> onInputStarted is fired
test__f
Attachment #8795084 - Flags: review?(mak77)
Depends on: 1306820
No longer depends on: 1306820
Blocks: 1306820
Drew, since I'll be spotty available for a couple days, and I have other reviews in queue, would you mind taking care of the omnibox APIs reviews? I don't want to slow down this effort too much due to a long queue.
This may also have some relevance to the work you are doing for the new integration API.

The specs are simple, an add-on can register a "magic" word with autocomplete, when that magic word + some search term is written in autocomplete, the add-on takes over autocomplete entries (or a part of them, see https://bugzilla.mozilla.org/show_bug.cgi?id=1166831#c9) and returns its own.
The UI will be similar to the seitch to tab ui, we show the addon name in the locationbar as we show "switch to tab" and we use the generic add-ons icon to identify entries (again, see https://bugzilla.mozilla.org/show_bug.cgi?id=1166831#c9 for open questions).
Flags: needinfo?(adw)
note if only the magic word or magic word + space is typed, we don't pass the control (at least for now).
Comment on attachment 8795085 [details]
Bug 1267810 - Use the new module to implement the omnibox api.

https://reviewboard.mozilla.org/r/81256/#review81428

::: browser/components/extensions/ext-omnibox.js:36
(Diff revision 3)
> +extensions.registerSchemaAPI("omnibox", "addon_parent", context => {
> +  let {extension} = context;
> +  return {
> +    omnibox: {
> +      onInputStarted: new EventManager(context, "omnibox.onInputStarted", fire => {
> +        extension.on(ExtensionSearchHandler.MSG_INPUT_STARTED, fire);

I'll update this in the next revision to wrap `fire` with an empty function, because, if I just pass in `fire` to extension.on(...), `onInputStarted` will get called with the name of the event as the first argument.
Attachment #8795085 - Flags: review?(aswan)
Hey Andrew, I'm mainly looking for general feedback on the overall implementation (e.g. how I've designed ExtensionSearchHandler.jsm, and how I'm interacting with UnifiedComplete.js, autocomplete.xml, and urlbarBindings.xml to display and handle the search suggestions.  

Let me know if you have any questions/suggestions.  Thanks!
(In reply to Matthew Wein [:K-9, :mattw] from comment #16)
> Hey Andrew, I'm mainly looking for general feedback on the overall
> implementation (e.g. how I've designed ExtensionSearchHandler.jsm, and how
> I'm interacting with UnifiedComplete.js, autocomplete.xml, and
> urlbarBindings.xml to display and handle the search suggestions.  
> 
> Let me know if you have any questions/suggestions.  Thanks!

I'm not familiar with any of the code that you changed in the first patch, I think somebody who has worked on it would probably be a better choice to give you feedback.
(In reply to Andrew Swan [:aswan] from comment #17)
> (In reply to Matthew Wein [:K-9, :mattw] from comment #16)
> > Hey Andrew, I'm mainly looking for general feedback on the overall
> > implementation (e.g. how I've designed ExtensionSearchHandler.jsm, and how
> > I'm interacting with UnifiedComplete.js, autocomplete.xml, and
> > urlbarBindings.xml to display and handle the search suggestions.  
> > 
> > Let me know if you have any questions/suggestions.  Thanks!
> 
> I'm not familiar with any of the code that you changed in the first patch, I
> think somebody who has worked on it would probably be a better choice to
> give you feedback.

Oops, sorry, that was directed at Drew... these names are a bit confusing :)
Comment on attachment 8795084 [details]
Bug 1267810 - Add a module for registering keywords and handling keyword input sessions.

https://reviewboard.mozilla.org/r/81254/#review82676

Everything here looks good to me except for the part in UnifiedComplete that calls into ExtensionSearchHandler.  There are a couple of aspects.

If I'm reading the patch right (maybe not, let me know), all the searches that are normally performed by UnifiedComplete are gated on the results of all the extensions.  There are several types of searches that UnifiedComplete does: bookmarks/history, open tabs, suggestions from the current search engine, and synced/remote tabs.  So all those searches would not even start until all extensions have returned their results.  That's not so good, even if extensions have only a small window where they have to reply.

I think you're going to have to modify the Search prototype so that it asks extensions for their results but does not wait for them to answer before starting all the other types of searches.  You can see how Search.prototype.execute starts the search engine suggestions fetch [1], and then the SQL-related fetches (bookmarks/history and open tabs) [2], and then remote tabs [3], but doesn't wait for them to finish.  Instead it adds results as they come in.

[1] https://dxr.mozilla.org/mozilla-central/source/toolkit/components/places/UnifiedComplete.js#916
[2] https://dxr.mozilla.org/mozilla-central/source/toolkit/components/places/UnifiedComplete.js#922
[3] https://dxr.mozilla.org/mozilla-central/source/toolkit/components/places/UnifiedComplete.js#928

Which leads to the second thing I'm not so sure about: how should results from extensions be interleaved with the other types of results?  Is that defined somewhere?  It's a UX question, but we could just punt and add them as they come in, regardless of what other types of results have or have not been added yet.
Attachment #8795084 - Flags: review?(adw)
Flags: needinfo?(adw)
(In reply to Drew Willcoxon :adw from comment #19)
> Comment on attachment 8795084 [details]
> WIP - Bug 1267810 - Add a module for registering magic words and handling
> searches.
> 
> https://reviewboard.mozilla.org/r/81254/#review82676
> 
> Everything here looks good to me except for the part in UnifiedComplete that
> calls into ExtensionSearchHandler.  There are a couple of aspects.
> 
> If I'm reading the patch right (maybe not, let me know), all the searches
> that are normally performed by UnifiedComplete are gated on the results of
> all the extensions.  There are several types of searches that
> UnifiedComplete does: bookmarks/history, open tabs, suggestions from the
> current search engine, and synced/remote tabs.  So all those searches would
> not even start until all extensions have returned their results.  That's not
> so good, even if extensions have only a small window where they have to
> reply.
> 
> I think you're going to have to modify the Search prototype so that it asks
> extensions for their results but does not wait for them to answer before
> starting all the other types of searches.  You can see how
> Search.prototype.execute starts the search engine suggestions fetch [1], and
> then the SQL-related fetches (bookmarks/history and open tabs) [2], and then
> remote tabs [3], but doesn't wait for them to finish.  Instead it adds
> results as they come in.

Yeah, that makes a lot more sense. I did some more testing in Chrome and this is how it works there. The suggestions from the extension are shown as they come in and the extension can continue to add suggestions as long as the navigation bar is in focus and the input hasn't changed.

> [1]
> https://dxr.mozilla.org/mozilla-central/source/toolkit/components/places/
> UnifiedComplete.js#916
> [2]
> https://dxr.mozilla.org/mozilla-central/source/toolkit/components/places/
> UnifiedComplete.js#922
> [3]
> https://dxr.mozilla.org/mozilla-central/source/toolkit/components/places/
> UnifiedComplete.js#928
> 
> Which leads to the second thing I'm not so sure about: how should results
> from extensions be interleaved with the other types of results?  Is that
> defined somewhere?  It's a UX question, but we could just punt and add them
> as they come in, regardless of what other types of results have or have not
> been added yet.

It's not documented afaict, but in Chrome the results from the extension always come first. When the suggestions come in from the extension, the other suggestions are bumped down to make room, and the ones at the end are removed to make the total always equal to 6 (Chrome never shows more than 6 suggestions).
(In reply to Matthew Wein [:K-9, :mattw] from comment #20)
> It's not documented afaict, but in Chrome the results from the extension
> always come first. When the suggestions come in from the extension, the
> other suggestions are bumped down to make room, and the ones at the end are
> removed to make the total always equal to 6 (Chrome never shows more than 6
> suggestions).

I think this makes sense, since the action from the user is explicit (I WANT results from this extension) we don't have lots of things to think about when mixing up stuff, I think we should just put these results at the top.

The problem, at a maximum, could be how we handle the heuristic result (search with/visit/autofill).
We may need at least to know immediately if the first word is a magic keyword, so we don't even build an heuristic result, and then fetch results asynchronously.
That means you likely need to build a cache of all the magic words and keep it updated so that we can quickly get an answer.
(In reply to Marco Bonardo [::mak] from comment #21)
> We may need at least to know immediately if the first word is a magic
> keyword, so we don't even build an heuristic result, and then fetch results
> asynchronously.
> That means you likely need to build a cache of all the magic words and keep
> it updated so that we can quickly get an answer.

That's something else I wanted to comment on: ExtensionSearchHandler.handleSearch is O(number of extensions).  Ideally it would be O(1).  Are magic words really only single words?  If so, we should be able to just grab the first space-delimited word in the input and look it up in the magic word map.
Depends on: 1309047
> ExtensionSearchHandler.handleSearch is O(number of extensions).  Ideally it
> would be O(1).  Are magic words really only single words?  If so, we should
> be able to just grab the first space-delimited word in the input and look it
> up in the magic word map.

Yeah, that's a good idea. I updated this in the latest revision to be O(1).
(In reply to Marco Bonardo [::mak] from comment #21)
> (In reply to Matthew Wein [:K-9, :mattw] from comment #20)
> > It's not documented afaict, but in Chrome the results from the extension
> > always come first. When the suggestions come in from the extension, the
> > other suggestions are bumped down to make room, and the ones at the end are
> > removed to make the total always equal to 6 (Chrome never shows more than 6
> > suggestions).
> 
> I think this makes sense, since the action from the user is explicit (I WANT
> results from this extension) we don't have lots of things to think about
> when mixing up stuff, I think we should just put these results at the top.
> 
> The problem, at a maximum, could be how we handle the heuristic result
> (search with/visit/autofill).
> We may need at least to know immediately if the first word is a magic
> keyword, so we don't even build an heuristic result, and then fetch results
> asynchronously.
> That means you likely need to build a cache of all the magic words and keep
> it updated so that we can quickly get an answer.

Sounds good. I noticed recently that the omnibox API in Chrome provides its own heuristic result in addition to the suggestions provided by the extension, so I think we should do this as well. In the latest revision, I added a heuristic result which follows the same pattern that Chrome does.  The heuristic result shows the extension name as the comment, and the current search string as the content.
Also, to be compatible with Chrome, in the latest revision I limited the number of suggestions an extension can provide for a given search string to 6. Depending on what UX suggests in response to bug 1166831, comment 10, however, we can obviously update this limitation or remove it entirely.
Attachment #8795085 - Flags: review?(aswan)
Attachment #8795084 - Flags: review?(adw)
Summary: [Omnibox] Add support for chrome.omnibox.onInputStarted → [Omnibox] Complete the implementation of chrome.omnibox
Component: WebExtensions: Untriaged → WebExtensions: Frontend
Priority: -- → P2
No longer blocks: 1267814
No longer blocks: 1267811
No longer blocks: 1267813
Blocks: 1267813, 1303905
No longer blocks: 1267813
No longer blocks: 1306820
Some notes on the current revision:

The URL bar:
 - Whenever a suggestion provided by a magic word is selected, the URL bar will currently show
     <grey default extension puzzle icon><"Extension: "><magic word><space><user input>
   - I'm using "Extension: " here as a placeholder until a decision is made. https://bugzilla.mozilla.org/show_bug.cgi?id=1306820#c4 suggests we use "Addon: ", and Chrome uses the the extension name instead.
   - A small concern I have with using "Addon: " is that it doesn't provide an indication of which extension is active. However, it does make it very clear that the results are coming from an add-on and not Firefox. On the other hand, I think using "Addon: " might be fine if the heuristic result always shows the extension's name.

The heuristic result:
 - For this result, I currently always show the extension name followed by a description. If the user sets the default suggestion using omnibox.setDefaultSuggestion, the description comes from the extension. Otherwise, the description is set to the current search string

Chrome differences:
 - Chrome hides the magic word whenever it is active. Right now I'm currently always showing the magic word, in the url bar and in each suggestion, because I'm not comfortable hiding it before bug 1309047 lands.

Testing:
 - The tests in the first patch test the module to confirm that:
    - the correct errors are thrown.
    - the correct events are emitted.
    - the correct suggestions are shown.
    - multiple registered magic words are handled correctly.
 - The tests in the second patch confirm that:
    - typing text into the url bar triggers the correct listeners.
    - the listeners are fired with the correct information.
Attached image Omnibox UI Screenshot (obsolete) —
Attached image Omnibox Chrome UI Screenshot (obsolete) —
Hey Markus and Daniel, do you think you could weigh in on what we should display in the URL bar and how we should display it when a magic word is active? I've attached an image to the bug of how it currently looks in the latest revision and how it looks in the latest version of Chrome. I've also gone into more detail on this in comment 41 under "The URL bar" and "Chrome differences".

Thanks!
Flags: needinfo?(mjaritz)
Flags: needinfo?(dveditz)
Comment on attachment 8795084 [details]
Bug 1267810 - Add a module for registering keywords and handling keyword input sessions.

https://reviewboard.mozilla.org/r/81254/#review85244

This looks good to me, thanks.  I do have one question about the handleCommand change.  It doesn't look right to me, but r+ with that resolved -- either let me know why it makes sense and works that way, or return early like you were before.

Also, it looks like your text editor maybe automatically stripped trailing whitespace in browser.dtd, xpcshell.ini, and autocomplete.xml?  Could you please revert those changes so that blame is preserved?

::: browser/base/content/urlbarBindings.xml:456
(Diff revision 10)
> +              case "extension":
> +                // Give the extension control of handling the command.
> +                let searchString = action.params.content;
> +                let magicWord = action.params.magicWord;
> +                this.ExtensionSearchHandler.handleInputEntered(magicWord, searchString, where);
> +                url = "";

I don't understand this part here.  Why set url to the empty string and then try to load it?  Shouldn't the method return at this point like your earlier revisions did?

::: toolkit/components/places/ExtensionSearchHandler.jsm:70
(Diff revision 10)
> +  },
> +
> +  /**
> +   * @return {boolean} true if there is an input session is currently active.
> +   */
> +  hasActiveInputSession() {

Nit: This could be `get hasActiveInputSession()` instead of a function, but no big deal.

::: toolkit/components/places/UnifiedComplete.js:978
(Diff revision 10)
>    *_matchFirstHeuristicResult(conn) {
>      // We always try to make the first result a special "heuristic" result.  The
>      // heuristics below determine what type of result it will be, if any.
>  
>      if (this._searchTokens.length > 0) {
> +      // The first search token is a magic word registered by an extension.

Nit: Please say "may be a magic word".  That would better match the other similar comments in this method, and at least for me, it's confusing when the comment above an `if` is unequivocal.

::: toolkit/components/places/tests/unifiedcomplete/test_extensionmatches.js:1
(Diff revision 10)
> +/* -*- indent-tabs-mode: nil; js-indent-level: 2 -*-

Nit: Could you please use camel case or an underscore  in this filename to make it easier to read?  i.e. extensionMatches or extension_matches
Attachment #8795084 - Flags: review?(adw) → review+
Comment on attachment 8795085 [details]
Bug 1267810 - Use the new module to implement the omnibox api.

https://reviewboard.mozilla.org/r/81256/#review85278

I haven't yet gone through the test but there are a few other things, listed below that I think are important to fix before this lands.  I'll take a look at the test in the next round when those get addressed.

::: browser/components/extensions/ext-omnibox.js:14
(Diff revision 10)
> +                                  "resource://gre/modules/ExtensionSearchHandler.jsm");
> +var {
> +  EventManager,
> +} = ExtensionUtils;
> +
> +let keyword = null;

This won't work, I think you want a map of extension to keyword.
Also, nothing here seems to handle the case of two extensions loading the same keyword.  I'm not even sure what the right behavior is (make loading the second extension fail?) but I think it needs to be addressed.

::: browser/components/extensions/ext-omnibox.js:33
(Diff revision 10)
> +
> +extensions.registerSchemaAPI("omnibox", "addon_parent", context => {
> +  let {extension} = context;
> +  return {
> +    omnibox: {
> +      onInputStarted: new EventManager(context, "omnibox.onInputStarted", fire => {

We've been trying to use SingletonEventManager everywhere

::: browser/components/extensions/schemas/omnibox.json:17
(Diff revision 10)
> +            "type": "object",
> +            "additionalProperties": { "$ref": "UnrecognizedProperty" },
> +            "properties": {
> +              "keyword": {
> +                "type": "string",
> +                "optional": true

is this really optional?  what does it mean to have an omnibox object in the manifest without a keyword property?
also, this should probably have a format constraining it -- can i make "http://" be my keyword?  can a keyword contain whitespace?
Attachment #8795085 - Flags: review?(aswan)
Comment on attachment 8795085 [details]
Bug 1267810 - Use the new module to implement the omnibox api.

https://reviewboard.mozilla.org/r/81256/#review85278

> This won't work, I think you want a map of extension to keyword.
> Also, nothing here seems to handle the case of two extensions loading the same keyword.  I'm not even sure what the right behavior is (make loading the second extension fail?) but I think it needs to be addressed.

Ah, you're right, thanks!

The module throws an error if an extension tries to register a keyword that's already registered, but we should also make sure it's handled correctly here. In Chrome, the most recently installed extension takes control, which I think is reasonable. Do you think we should go for Chrome compatibility here or do something different?

> We've been trying to use SingletonEventManager everywhere

Okay, I'll update this.

> is this really optional?  what does it mean to have an omnibox object in the manifest without a keyword property?
> also, this should probably have a format constraining it -- can i make "http://" be my keyword?  can a keyword contain whitespace?

Yeah that's a good point, I removed the "optional" keyword.

Also, thanks for noticing this, I copied over the manifest from Chrome which doesn't have any contstraints. However, in Chrome, certain strings don't work - from my own testing, the API is broken if the keyword contains a space or a colon, starts with a question mark, or ends with a forward slash.  Here are some examples:

Keywords that work:
 - "*"
 - "http"
 - "消しゴム"
 - "f?a" (doesn't start with ?)
 - "fa?" (doesn't start with ?)
 - "f/x" (doesn't end with /)
 - "/fx" (doesn't end with /)

Keywords that don't work:
 - "?" (starts with ?)
 - "?fx" (starts with ?)
 - "/" (ends with /)
 - "fx/" (ends with /)
 - ":" (contains a colon)
 - "f:x" (contains a colon)
 - "fx:" (contains a colon)
 - "http://" (contains a colon)
 - " " (contains a space)
 - "f x" (contains a space)

I think we should check for invalid keywords early on in the manifest. Do you think it sounds reasonable to add a `pattern` key to the manifest to disallow keywords from starting with "?", ending with "/", or containing either " " or ":"?  There may be other cases that I haven't thought of, but I think that would cover the really important ones.
Comment on attachment 8795085 [details]
Bug 1267810 - Use the new module to implement the omnibox api.

https://reviewboard.mozilla.org/r/81256/#review85278

> Okay, I'll update this.

I had to change it back because `SingletonEventManager` doesn't support `fire.withoutClone`, which I need to use in order to pass the callback for onInputChanged to the extension. Do you know of a workaround for this, or should I file a follow up bug?
Comment on attachment 8795084 [details]
Bug 1267810 - Add a module for registering keywords and handling keyword input sessions.

https://reviewboard.mozilla.org/r/81254/#review85244

Thanks for the review! I went through and made the changes you suggested.

I also made a few other changes so here's a summary of the what else changed:

Design changes:
 - renamed all instances of `magic word` to `keyword` since I think `keyword` is more appropriate.
 - made the input session's `description` default to the extension's name so that `getExtensionName()` could be removed. 
 - created a new class called `KeywordInfo` which each keyword gets associated to.  The InputSession class is now only used to keep track of the active input session.

Functional change:
 - fixed a small issue with the heuristic result. It turns out the heuristic result should always use the current search string for the content instead of letting the extension modify it. The extension should only be allowed to modify the comment.

> I don't understand this part here.  Why set url to the empty string and then try to load it?  Shouldn't the method return at this point like your earlier revisions did?

We spoke about this over IRC and you realized `this.handleRevert()` was what I was looking for. For other's reference, the urlbar needs to revert to the loaded url when a command is handled by the extension.

> Nit: This could be `get hasActiveInputSession()` instead of a function, but no big deal.

I'd like to keep it as a function here just because I think a getter would make more sense for retreiving the active input session and I'd rather not make it public if it doesn't need to be.

> Nit: Please say "may be a magic word".  That would better match the other similar comments in this method, and at least for me, it's confusing when the comment above an `if` is unequivocal.

Thanks for noticing this - fixed.

> Nit: Could you please use camel case or an underscore  in this filename to make it easier to read?  i.e. extensionMatches or extension_matches

Sure - I think an underscore makes sense in this case. I named this after test_tabmatches.js (there's a few others named similarly), so I filed bug 1311049 to handle renaming those tests to follow the same naming convention.
(In reply to Matthew Wein [:K-9, :mattw] from comment #51)
> I named this after test_tabmatches.js (there's a few others named similarly),
> so I filed bug 1311049 to handle renaming those tests to follow the same
> naming convention.

Ah, OK, thanks.  We don't really have any naming conventions anywhere for tests in the Firefox front-end unfortunately.  It's kind of a mish-mash.  :-/
Comment on attachment 8795085 [details]
Bug 1267810 - Use the new module to implement the omnibox api.

https://reviewboard.mozilla.org/r/81256/#review85704

::: browser/components/extensions/ext-omnibox.js:21
(Diff revision 12)
> +
> +/* eslint-disable mozilla/balanced-listeners */
> +extensions.on("manifest_omnibox", (type, directive, extension, manifest) => {
> +  let keyword = manifest.omnibox.keyword;
> +  if (ExtensionSearchHandler.isKeywordRegistered(keyword)) {
> +    ExtensionSearchHandler.unregisterKeyword(keyword);

I'm not sure what I think about this behavior (most recent wins) but even if its what we want, we have a problem here: if extension A registers for a keyword, then extension B registers from the same keyword and steals it, then extension A is disabled or uninstalled, it will unregister B's handler!

::: browser/components/extensions/schemas/omnibox.json:17
(Diff revision 12)
> +            "type": "object",
> +            "additionalProperties": { "$ref": "UnrecognizedProperty" },
> +            "properties": {
> +              "keyword": {
> +                "type": "string",
> +                "pattern": "(^[^?\\s:][^\\s:]+[^/\\s:]$)|(^[^?\\s:][^/\\s:]$)|(^[^?\\s:/*]$)"

I'm scratching my head about this, what is it meant to allow/deny?  Can you add tests that manifests with invalid keywords are rejected?
Attachment #8795085 - Flags: review?(aswan)
(In reply to Andrew Swan [:aswan] from comment #55)
> I'm not sure what I think about this behavior (most recent wins)

There's some risk add-ons may contend keywords, like if a keyword happens to be widely used, another add-on could decide to "hijack" it for its own purposes.
I think it would be better if the add-on winning is the one that was installed first by the user. Do we have that information at hand?
(In reply to Matthew Wein [:K-9, :mattw] from comment #44)
> Hey Markus and Daniel, do you think you could weigh in on what we should
> display in the URL bar and how we should display it when a magic word is
> active?

There are maybe too many uses for "example" in those examples. Does chrome really make the keyword (which I assume is "example") go away? Looks like it got moved inside the box with the "Omnibox" label. Or is that occurance of "Example" the name of the extension? I think I like keeping the magic keyword even if you do show the name of the extension in the box (which seems nice).

I'm not sure I'm keen on the icon and box type display. We use those when we have loaded pages (and there's a bug somewhere about what to show when we load an extension page) but in this case the changes refer to the suggestions and not the page content. Looks like all the results came from the extension, not interleaved with other awesomebar results? In that case could you just have plain words as typed in the URL bar and then have the first line of suggestions be "suggestions from <name of addon>". Or tack that onto the end of the suggestion, the way currently " -- Search with DuckDuckGo" is put on the end of my searches?

Tossing this needinfo? to Tanvi since there's overlap with her work on the Site Identity box.
Flags: needinfo?(dveditz) → needinfo?(tanvi)
Comment on attachment 8795085 [details]
Bug 1267810 - Use the new module to implement the omnibox api.

https://reviewboard.mozilla.org/r/81256/#review85704

I'm not sure but I think you may have missed my responses in the last revision (comments 47 and 48 in bugzilla).
Attachment #8801833 - Attachment is obsolete: true
Attachment #8801837 - Attachment is obsolete: true
(In reply to Daniel Veditz [:dveditz] from comment #57)
> (In reply to Matthew Wein [:K-9, :mattw] from comment #44)
> > Hey Markus and Daniel, do you think you could weigh in on what we should
> > display in the URL bar and how we should display it when a magic word is
> > active?
> 
> There are maybe too many uses for "example" in those examples. Does chrome
> really make the keyword (which I assume is "example") go away? Looks like it
> got moved inside the box with the "Omnibox" label. Or is that occurance of
> "Example" the name of the extension? I think I like keeping the magic
> keyword even if you do show the name of the extension in the box (which
> seems nice).
Chrome does make the keyword go away - it only puts the extension name in the label. I've attached two new screenshots which should display this information better. I also think keeping the magic word might be a good idea, and it's something we can fairly easily remove later on in a follow-up if we decide it's better that way.

> I'm not sure I'm keen on the icon and box type display. We use those when we
> have loaded pages (and there's a bug somewhere about what to show when we
> load an extension page) but in this case the changes refer to the
> suggestions and not the page content. 
I followed the same pattern as "Switch to tab" suggestions, but those use the action text "Switch to tab: " as the label.  So I think it would be more fitting to use something along the lines of "Send to <extension name>: " but that could get rather long if an extension has a long name, so I'm thinking "Send to add-on: " might be better.

> Looks like all the results came from the extension, not interleaved with other
> awesomebar results? In that case could you just have plain words as typed in the URL bar and then
> have the first line of suggestions be "suggestions from <name of addon>". Or
> tack that onto the end of the suggestion, the way currently " -- Search with
> DuckDuckGo" is put on the end of my searches?
Other results can still show up (you can see this in the new screenshots I've uploaded) but the extension's suggestions always come first. I think this is a good idea, but we may want to word it differently since the extension can also handle what happens when a suggestion is clicked on. I'm thinking something like "send search string to <name of extension>" but there's probably a better wording.
Comment on attachment 8795085 [details]
Bug 1267810 - Use the new module to implement the omnibox api.

https://reviewboard.mozilla.org/r/81256/#review85278

> Ah, you're right, thanks!
> 
> The module throws an error if an extension tries to register a keyword that's already registered, but we should also make sure it's handled correctly here. In Chrome, the most recently installed extension takes control, which I think is reasonable. Do you think we should go for Chrome compatibility here or do something different?

We decided to go for Chrome compatibility here.

> I had to change it back because `SingletonEventManager` doesn't support `fire.withoutClone`, which I need to use in order to pass the callback for onInputChanged to the extension. Do you know of a workaround for this, or should I file a follow up bug?

`runSafeSyncWithoutClone` is the workaround `for fire.withoutClone`
Comment on attachment 8795085 [details]
Bug 1267810 - Use the new module to implement the omnibox api.

https://reviewboard.mozilla.org/r/81256/#review85704

> I'm not sure what I think about this behavior (most recent wins) but even if its what we want, we have a problem here: if extension A registers for a keyword, then extension B registers from the same keyword and steals it, then extension A is disabled or uninstalled, it will unregister B's handler!

I think we should stick with this behavior for Chrome compat.

This issue can be fixed by removing extension A from the keyword map when extension B is installed.

> I'm scratching my head about this, what is it meant to allow/deny?  Can you add tests that manifests with invalid keywords are rejected?

I simplified this to (^[^?\s:][^\s:]*[^/\s:]$)|(^[^?\s:/*]$) and when you split it up into choices it becomes:
choice 1) ^[^?\s:][^\s:]*[^/\s:]$ 
choice 2) ^[^?\s:/*]$
I'll add tests to verify it works as expected.
(In reply to Matthew Wein [:K-9, :mattw] from comment #44)
> Hey Markus and Daniel, do you think you could weigh in on what we should
> display in the URL bar and how we should display it when a magic word is
> active? I've attached an image to the bug of how it currently looks in the
> latest revision and how it looks in the latest version of Chrome. I've also
> gone into more detail on this in comment 41 under "The URL bar" and "Chrome
> differences".
> 
> Thanks!

I do not feel comfortable just weighing in on one small part of the feature, without taking time to understand the feature and it's different flows. Is there a designer working on this? I've only seen one question by :shorlander in the tracking bug. (https://bugzilla.mozilla.org/show_bug.cgi?id=1166831#c7)

What are the use-cases for this feature?
How popular is it? (with Chrome)
What are examples on Chrome? 
Is there a way to explore the current implementation in Firefox?
Flags: needinfo?(mjaritz) → needinfo?(mwein)
(In reply to Markus Jaritz [:maritz] (UX) from comment #67)
> (In reply to Matthew Wein [:K-9, :mattw] from comment #44)
> > Hey Markus and Daniel, do you think you could weigh in on what we should
> > display in the URL bar and how we should display it when a magic word is
> > active? I've attached an image to the bug of how it currently looks in the
> > latest revision and how it looks in the latest version of Chrome. I've also
> > gone into more detail on this in comment 41 under "The URL bar" and "Chrome
> > differences".
> > 
> > Thanks!
> 
> I do not feel comfortable just weighing in on one small part of the feature,
> without taking time to understand the feature and it's different flows. Is
> there a designer working on this? I've only seen one question by :shorlander
> in the tracking bug.
> (https://bugzilla.mozilla.org/show_bug.cgi?id=1166831#c7)

Can you recommend someone who does feel comfortable talking about the awesomebar and how we present mode information? Would that be :shorlander instead? The question is centered around whether we're applying the proper style for activating a developer-focused mode from the awesomebar. I know there's been a number of discussions around styling of the mode indicator, and if you could redirect to who's on point for that it'd be great.

Thanks Markus!
Flags: needinfo?(mjaritz)
(In reply to Kev Needham [:kev] from comment #68)
> Can you recommend someone who does feel comfortable talking about the
> awesomebar and how we present mode information? Would that be :shorlander
> instead?

Yes, :shorlander has done the most work on awesomebar.
http://people.mozilla.org/~shorlander/mockups/AwesomeResults/AwesomeResults-02.html
He however has similar questions to mine.
https://bugzilla.mozilla.org/show_bug.cgi?id=1166831#c7

@:shorlander do you have an answer for :mattw?
If not, we can put it in our add-on ux backlog.
(In reply to Matthew Wein [:K-9, :mattw] from comment #44)
> Hey Markus and Daniel, do you think you could weigh in on what we should
> display in the URL bar and how we should display it when a magic word is
> active? I've attached an image to the bug of how it currently looks in the
> latest revision and how it looks in the latest version of Chrome. I've also
> gone into more detail on this in comment 41 under "The URL bar" and "Chrome
> differences".
> 
> Thanks!
Flags: needinfo?(mjaritz) → needinfo?(shorlander)
> What are the use-cases for this feature?

For add-ons to be able to add themselves to the omnibar. This is important for Chrome parity and for add-ons that currently play with the awesome bar in many ways: https://addons.mozilla.org/en-us/firefox/search/?q=awesome&appver=49.0&platform=mac

> How popular is it? (with Chrome)

https://github.com/andymckay/arewewebextensionsyet.com/blob/master/usage.csv#L76

It is a useful API and one I feel we should implement.

> What are examples on Chrome? 

Here's a list of over 250 extensions using it: https://public.etherpad-mozilla.org/p/omnibox-extensions

> Is there a way to explore the current implementation in Firefox?

Aside from applying Matts patches and build Firefox, probably not.
Comment on attachment 8795085 [details]
Bug 1267810 - Use the new module to implement the omnibox api.

https://reviewboard.mozilla.org/r/81256/#review87142

::: browser/components/extensions/ext-omnibox.js:6
(Diff revision 14)
> +/* -*- Mode: indent-tabs-mode: nil; js-indent-level: 2 -*- */
> +/* vim: set sts=2 sw=2 et tw=80: */
> +"use strict";
> +
> +Cu.import("resource://gre/modules/ExtensionUtils.jsm");
> +Cu.import("resource://gre/modules/Task.jsm");

this doesn't appear to actually be used?

::: browser/components/extensions/ext-omnibox.js:16
(Diff revision 14)
> +  runSafeSyncWithoutClone,
> +  SingletonEventManager,
> +} = ExtensionUtils;
> +
> +// WeakMap[extension -> keyword]
> +let gKeywordMap = new WeakMap();

no g prefix in webextensions code please

::: browser/components/extensions/ext-omnibox.js:31
(Diff revision 14)
> +/* eslint-disable mozilla/balanced-listeners */
> +extensions.on("manifest_omnibox", (type, directive, extension, manifest) => {
> +  let keyword = manifest.omnibox.keyword;
> +  // Check to see if an extension already has registered this keyword.  If so,
> +  // unregister it as the user probably wants this new extension to have it.
> +  if (ExtensionSearchHandler.isKeywordRegistered(keyword)) {

Is ExtensionSearchHandler meant to be used exclusively by webextensions?  If not, this isn't really safe (other users of that module won't be notified if something usurps their keyword and webextensions won't get cleaned up here if something else takes their keyword).
I still think the first extension to register a keyword should win (and I think Marco agreed in a separate comment on the bug)

::: browser/components/extensions/ext-omnibox.js:45
(Diff revision 14)
> +
> +extensions.on("shutdown", (type, extension) => {
> +  let keyword = gKeywordMap.get(extension);
> +  if (gKeywordMap.has(keyword)) {
> +    ExtensionSearchHandler.unregisterKeyword(keyword);
> +  }

You're using a WeakMap but still for thoroughness, remove the entry from the map here?

::: browser/components/extensions/ext-omnibox.js:54
(Diff revision 14)
> +extensions.registerSchemaAPI("omnibox", "addon_parent", context => {
> +  let {extension} = context;
> +  return {
> +    omnibox: {
> +      setDefaultSuggestion(suggestion) {
> +        let keyword = gKeywordMap.get(extension);

Should this verify that there actually is an entry here?  ie, in the scenario where multiple extensions are trying to use the same keyword, this extension might not actually "own" the keyword in which case i think you'll call setDefaultSuggestion with null or undefined as the first parameter.  I don't know what happens if you do that but you probably shouldn't do it.

::: browser/components/extensions/ext-omnibox.js:81
(Diff revision 14)
> +        };
> +      }).api(),
> +
> +      onInputChanged: new SingletonEventManager(context, "omnibox.onInputChanged", fire => {
> +        let listener = (eventName, searchString, suggestCallback) => {
> +          runSafeSyncWithoutClone(fire, searchString, suggestCallback);

I don't know if there are cross-compartment issues here but this certainly won't work cross-process.  How is this passing tests?
Attachment #8795085 - Flags: review?(aswan)
Comment on attachment 8795085 [details]
Bug 1267810 - Use the new module to implement the omnibox api.

https://reviewboard.mozilla.org/r/81256/#review87142

> this doesn't appear to actually be used?

good catch - removed.

> no g prefix in webextensions code please

I'd prefer to keep it because I think it helps to distinguish global variables from other variables, and this prefix is already used in ext-contextMenus.js:
https://dxr.mozilla.org/mozilla-central/source/browser/components/extensions/ext-contextMenus.js

> Is ExtensionSearchHandler meant to be used exclusively by webextensions?  If not, this isn't really safe (other users of that module won't be notified if something usurps their keyword and webextensions won't get cleaned up here if something else takes their keyword).
> I still think the first extension to register a keyword should win (and I think Marco agreed in a separate comment on the bug)

That's a good point. I think this might end up getting used by others in the future. I'll go with the recommendation that you and Marco gave to let the first extension to register a keyword win. If later on we decide to change this we can add in the code to provide the necessary notifications when keywords are unregistered.

> You're using a WeakMap but still for thoroughness, remove the entry from the map here?

Done.

> Should this verify that there actually is an entry here?  ie, in the scenario where multiple extensions are trying to use the same keyword, this extension might not actually "own" the keyword in which case i think you'll call setDefaultSuggestion with null or undefined as the first parameter.  I don't know what happens if you do that but you probably shouldn't do it.

In that case the module would throw an error. I decided to go with the recommendation you and Marco gave to let the first extension to register a keyword win, so at this point the keyword will always exist.

> I don't know if there are cross-compartment issues here but this certainly won't work cross-process.  How is this passing tests?

This wasn't an issue before the cross-compartment code landed but it definitely is now. Rob helped me figure out how to update this so I'll make those changes in the next revision and ask Rob to provide feedback.
Comment on attachment 8795085 [details]
Bug 1267810 - Use the new module to implement the omnibox api.

https://reviewboard.mozilla.org/r/81256/#review87448

This is getting close.  How about a test of multiple extensions registering the same keyword though?

::: browser/components/extensions/ext-c-omnibox.js:7
(Diff revision 15)
> +/* vim: set sts=2 sw=2 et tw=80: */
> +"use strict";
> +
> +Cu.import("resource://gre/modules/ExtensionUtils.jsm");
> +
> +XPCOMUtils.defineLazyModuleGetter(this, "ExtensionSearchHandler",

this isn't used here

::: browser/components/extensions/ext-omnibox.js:22
(Diff revision 15)
> +let gSuggestionsCallbackMap = new Map();
> +
> +/* eslint-disable mozilla/balanced-listeners */
> +extensions.on("manifest_omnibox", (type, directive, extension, manifest) => {
> +  let keyword = manifest.omnibox.keyword;
> +  ExtensionSearchHandler.registerKeyword(keyword, extension);

This can throw, you should catch here and perhaps put a nicely formatted message into the console explaning what happened.

::: browser/components/extensions/ext-omnibox.js:28
(Diff revision 15)
> +  ExtensionSearchHandler.unregisterKeyword(keyword);
> +  gSuggestionsCallbackMap.delete(keyword);
> +  gKeywordMap.delete(extension);

This should all be inside something like `if (keyword)`

::: browser/components/extensions/ext-omnibox.js:40
(Diff revision 15)
> +  let {extension} = context;
> +  return {
> +    omnibox: {
> +      setDefaultSuggestion(suggestion) {
> +        let keyword = gKeywordMap.get(extension);
> +        ExtensionSearchHandler.setDefaultSuggestion(keyword, suggestion);

You still need to check that you have a valid keyword here.  If this extension came after another extension (or if an extension calls this method without having a valid omnibox section in their manifest) then this will throw and the caller will just see an internal error.  Please detect that and return an appropriate error to the caller.

::: browser/components/extensions/test/browser/browser_ext_omnibox.js:46
(Diff revision 15)
> +        }
> +      });
> +    },
> +  });
> +
> +  function synthesizeKeyWithDelay(key, event) {

I don't think any caller ever passes something other than `{}` for the second argument, can you just remove it?

::: browser/components/extensions/test/browser/browser_ext_omnibox.js:49
(Diff revision 15)
> +  });
> +
> +  function synthesizeKeyWithDelay(key, event) {
> +    return new Promise(resolve => {
> +      EventUtils.synthesizeKey(key, event);
> +      setTimeout(resolve, 100);

what's this for?

::: browser/components/extensions/test/browser/browser_ext_omnibox.js:55
(Diff revision 15)
> +    });
> +  };
> +
> +  function* expectEvent(event, expected = {}) {
> +    let actual = yield extension.awaitMessage(event);
> +    ok(event, `Expected "${event}"" to have fired`);

event is a string here, just make this `ok(true, ...` or even just `info(...)`

::: browser/components/extensions/test/browser/browser_ext_omnibox.js:58
(Diff revision 15)
> +  function* expectEvent(event, expected = {}) {
> +    let actual = yield extension.awaitMessage(event);
> +    ok(event, `Expected "${event}"" to have fired`);
> +    if (expected.text) {
> +      is(actual.text, expected.text,
> +        ` - with text: "${expected.text}".`);

Please make the message here self-contained so when something fails and we see an excerpt we don't have to go find the context to understand what failed.
same below.
Attachment #8795085 - Flags: review?(aswan)
Comment on attachment 8795085 [details]
Bug 1267810 - Use the new module to implement the omnibox api.

https://reviewboard.mozilla.org/r/81256/#review87448

Thanks for the quick review!

I added a test to confirm that an error is thrown when an extension tries to register a keyword that's already in use.

> this isn't used here

Thanks, removed.

> This can throw, you should catch here and perhaps put a nicely formatted message into the console explaning what happened.

If this throws, the ExtensionSearchHandler prints a user-friendly error message to the console: "The keyword provided is already registered: <keyword>" so I'd like to leave this as is, but I'll add a test for it.

> This should all be inside something like `if (keyword)`

Good catch, I added a comment explaining why this is needed.

> You still need to check that you have a valid keyword here.  If this extension came after another extension (or if an extension calls this method without having a valid omnibox section in their manifest) then this will throw and the caller will just see an internal error.  Please detect that and return an appropriate error to the caller.

Done.

> I don't think any caller ever passes something other than `{}` for the second argument, can you just remove it?

Done.

> what's this for?

Without a small delay the awesomebar doesn't have enough time to process and show the suggestions before I check them.

> event is a string here, just make this `ok(true, ...` or even just `info(...)`

Done.

> Please make the message here self-contained so when something fails and we see an excerpt we don't have to go find the context to understand what failed.
> same below.

Done.
Comment on attachment 8795084 [details]
Bug 1267810 - Add a module for registering keywords and handling keyword input sessions.

https://reviewboard.mozilla.org/r/81254/#review87644

Some drive-by comments (I haven't reviewed this in depth, I've only glanced over the relevant bits to understand the other patch).

::: toolkit/components/places/ExtensionSearchHandler.jsm:38
(Diff revision 14)
> +   * @param {Object} extension The extension registering the keyword.
> +   */
> +  registerKeyword(keyword, extension) {
> +    if (gKeywordMap.has(keyword)) {
> +      throw new Error(`The keyword provided is already registered: ${keyword}`);
> +      return;

`throw` already stops execution, `return` is superfluous.

I haven't looked at how the keywords are implemented, but what happens if the user has already defined a keyword for their bookmark and the extension attempts to register it?

::: toolkit/components/places/UnifiedComplete.js:1321
(Diff revision 14)
>      });
>    },
>  
> +  _matchExtensionSuggestions() {
> +    ExtensionSearchHandler.handleSearch(this._searchTokens[0], this._originalSearchString,
> +      suggestions => {

What if the extension takes a long while before suggesting a result, and the user has meanwhile modified the query?

This scenario is quite realistic, e.g. I have an omnibox extension that fetches suggestion results from an external API, and users usually type faster than that.

I suggest (no pun intended) to maintain a counter or original search term, and compare the current counter/search term with the counter/search term that accompanies the suggestion. If they do not match, ignore the suggestion.

I recommend the counter over a search term, because then you can be certain that the ID is unique, which will be very useful downstream (see my comments in your other patch).
Comment on attachment 8795085 [details]
Bug 1267810 - Use the new module to implement the omnibox api.

https://reviewboard.mozilla.org/r/81256/#review87656

::: browser/components/extensions/ext-c-omnibox.js:17
(Diff revision 16)
> +extensions.registerSchemaAPI("omnibox", "addon_child", context => {
> +  return {
> +    omnibox: {
> +      onInputChanged: new SingletonEventManager(context, "omnibox.onInputChanged", fire => {
> +        let listener = searchString => {
> +          runSafeSyncWithoutClone(fire, searchString, suggestions => {

If the console does not show meaningful errors (I didn't test), it wouldn't hurt to validate the type of suggestions here, to help developers with debugging.

::: browser/components/extensions/ext-omnibox.js:23
(Diff revision 16)
> +
> +/* eslint-disable mozilla/balanced-listeners */
> +extensions.on("manifest_omnibox", (type, directive, extension, manifest) => {
> +  let keyword = manifest.omnibox.keyword;
> +  ExtensionSearchHandler.registerKeyword(keyword, extension);
> +  gKeywordMap.set(extension, keyword);

`registerKeyword` can throw if the keyword was already known to `ExtensionSearchHandler`, which prevents the extension from loading. This is a big thing.

I think that it's better to gracefully ignore the registration and print an error and/or reject invocations to methods such as setDefaultSuggestion if the keyword is unavailable.

::: browser/components/extensions/ext-omnibox.js:48
(Diff revision 16)
> +        let keyword = gKeywordMap.get(extension);
> +        if (!keyword) {
> +          return Promise.reject("Failed to set the default suggestion");
> +        }
> +        ExtensionSearchHandler.setDefaultSuggestion(keyword, suggestion);
> +        return Promise.resolve();

This is the default return value, there is no need for `return Promise.resolve();`

::: browser/components/extensions/ext-omnibox.js:51
(Diff revision 16)
> +        }
> +        ExtensionSearchHandler.setDefaultSuggestion(keyword, suggestion);
> +        return Promise.resolve();
> +      },
> +
> +      addSuggestionsInternal(suggestions) {

See my comments below in `omnibox.onInputChangedInternal`.

You should pass the suggestion callback ID here, and use that ID to fetch the relevant callback from the global map.

The reason for using the ID here is to make sure that callbacks for old onInputChanged calls are ignored.

Alternatively, you can stick to the use of the keyword, but then you have to make sure that ExtensionSearchHandler ignores callbacks for old queries.

::: browser/components/extensions/ext-omnibox.js:98
(Diff revision 16)
> +      }).api(),
> +
> +      onInputChangedInternal: new SingletonEventManager(context, "omnibox.onInputChangedInternal", fire => {
> +        let listener = (eventName, searchString, suggestionsCallback) => {
> +          let keyword = gKeywordMap.get(extension);
> +          gSuggestionsCallbackMap.set(keyword, suggestionsCallback);

See my review at `_matchExtensionSuggestions` in your other patch.

You should pass down a unique value (e.g. a counter), and use this as the key for the map.

I will refer to this unique value as "suggestion callback ID" in the rest of my review, for clarity.

::: browser/components/extensions/ext-omnibox.js:103
(Diff revision 16)
> +          gSuggestionsCallbackMap.set(keyword, suggestionsCallback);
> +          fire(searchString);
> +        };
> +        extension.on(ExtensionSearchHandler.MSG_INPUT_CHANGED, listener);
> +        return () => {
> +          extension.off(ExtensionSearchHandler.MSG_INPUT_CHANGED, listener);

Do not forget to clean up the `gSuggestionsCallbackMap` here. Presently, when an extension page reloads, the callback will still be lingering in the global map.

To clean up, it suffices to delete the callback with the suggestion callback ID. This ID is only available in the "listener" callback, but you can create a variable in the same scope as "listener" to store *the* last known ID. There should be only one, because presumably there is only one omnibox/awesomebar.

Following that assumption, you should be able to delete the pending suggestion callback (if any) before setting a new one in the listener above.

::: browser/components/extensions/test/browser/browser_ext_omnibox.js:21
(Diff revision 16)
> +      browser.omnibox.onInputStarted.addListener(() => {
> +        browser.test.sendMessage("on-input-started-fired");
> +      });
> +
> +      browser.omnibox.onInputChanged.addListener((text, suggest) => {
> +        browser.test.sendMessage("on-input-changed-fired", {text});

It seems that this test event is used in `startInput*`, and that its callers expect a search suggestion to be ready. To avoid any timing issues (e.g. if somehow IPC is so busy that the main process receives and processes the `test.sendMessage` call before `suggest` is called), move the `test.sendMessage` call after `suggest`.

::: browser/components/extensions/test/browser/browser_ext_omnibox.js:49
(Diff revision 16)
> +  });
> +
> +  function synthesizeKeyWithDelay(key) {
> +    return new Promise(resolve => {
> +      EventUtils.synthesizeKey(key, {});
> +      setTimeout(resolve, 100);

Please add a comment that explains the meaning of this arbitrary number.

::: browser/components/extensions/test/browser/browser_ext_omnibox.js:213
(Diff revision 16)
> +  SimpleTest.endMonitorConsole();
> +  yield waitForConsole;
> +
> +  yield extension2.unload();
> +  yield extension.unload();
> +});

Please add tests for the following scenarios:

- suggest is called asynchronously.
- suggest is called twice (expected: the results stack up).
- suggest is called twice with a delay between the two calls (same expectation).
- suggest is called after the user has triggered more input (expected: the previous call is ignored).

::: browser/components/extensions/test/xpcshell/test_ext_manifest_omnibox.js:37
(Diff revision 16)
> +
> +  // rejected single character keywords
> +  yield testKeyword({keyword: "?", expectError: true});
> +  yield testKeyword({keyword: " ", expectError: true});
> +  yield testKeyword({keyword: ":", expectError: true});
> +  yield testKeyword({keyword: "*", expectError: true});

Why is a single asterisk not allowed? The questionmark marks a search query, but AFAIK there is nothing special with asterisks.

::: browser/components/extensions/test/xpcshell/test_ext_manifest_omnibox.js:49
(Diff revision 16)
> +  yield testKeyword({keyword: "f?a", expectError: false});
> +  yield testKeyword({keyword: "fa?", expectError: false});
> +  yield testKeyword({keyword: "f/x", expectError: false});
> +  yield testKeyword({keyword: "/fx", expectError: false});
> +
> +  // rejected mult-character keywords

s/mult/multi/

::: browser/components/extensions/test/xpcshell/test_ext_manifest_omnibox.js:60
(Diff revision 16)
> +  yield testKeyword({keyword: "fx/", expectError: true});
> +  yield testKeyword({keyword: "f:x", expectError: true});
> +  yield testKeyword({keyword: "fx:", expectError: true});
> +  yield testKeyword({keyword: "f x", expectError: true});
> +
> +  // miscelaneous tests

s/miscelaneous/miscellaneous/
Comment on attachment 8795085 [details]
Bug 1267810 - Use the new module to implement the omnibox api.

https://reviewboard.mozilla.org/r/81256/#review87704

(clearing review - see above comments; looks good in general, but needs some more attention for edge cases)
Attachment #8795085 - Flags: review?(rob) → review-
Comment on attachment 8795084 [details]
Bug 1267810 - Add a module for registering keywords and handling keyword input sessions.

https://reviewboard.mozilla.org/r/81254/#review87644

> `throw` already stops execution, `return` is superfluous.
> 
> I haven't looked at how the keywords are implemented, but what happens if the user has already defined a keyword for their bookmark and the extension attempts to register it?

At the moment, the bookmark keyword will be ignored if the extension registers the same keyword. I think this would be a good case to handle in a follow-up bug.

> What if the extension takes a long while before suggesting a result, and the user has meanwhile modified the query?
> 
> This scenario is quite realistic, e.g. I have an omnibox extension that fetches suggestion results from an external API, and users usually type faster than that.
> 
> I suggest (no pun intended) to maintain a counter or original search term, and compare the current counter/search term with the counter/search term that accompanies the suggestion. If they do not match, ignore the suggestion.
> 
> I recommend the counter over a search term, because then you can be certain that the ID is unique, which will be very useful downstream (see my comments in your other patch).

The callback should stay active until either the input changes or input session ends. `onInputChanged` will always fire if the input changes, and `onInputEntered` or `onInputCancelled` will fire when the input session ends.  Therefore, what I'd like to do is invalidate the callback in the extension script once one of these three events fires. I'm not really sure how I would use a counter to do this.
Comment on attachment 8795084 [details]
Bug 1267810 - Add a module for registering keywords and handling keyword input sessions.

https://reviewboard.mozilla.org/r/81254/#review87644

> The callback should stay active until either the input changes or input session ends. `onInputChanged` will always fire if the input changes, and `onInputEntered` or `onInputCancelled` will fire when the input session ends.  Therefore, what I'd like to do is invalidate the callback in the extension script once one of these three events fires. I'm not really sure how I would use a counter to do this.

Thanks for clarifying this more over IRC, I've updated the latest revision to use a counter for the callback IDs.
Comment on attachment 8795084 [details]
Bug 1267810 - Add a module for registering keywords and handling keyword input sessions.

https://reviewboard.mozilla.org/r/81254/#review88666

::: toolkit/components/places/ExtensionSearchHandler.jsm:144
(Diff revisions 14 - 15)
> +      // we pass the extension the active callback ID.
> +      gCallbackID++;
> +      extension.emit(this.MSG_INPUT_CHANGED, text, gCallbackID, (suggestions, callbackID) => {
> +        // Only provide suggestions if the callback is still active.
> +        if (gCallbackID == callbackID) {
> +          callback(suggestions);

Just a thought: If there is only one callback active at any time, why don't you make it a method of ExtensionSearchHandler? That allows for a simpler implementation downstream because you don't need to track the callback, only the callback ID.
Comment on attachment 8795085 [details]
Bug 1267810 - Use the new module to implement the omnibox api.

https://reviewboard.mozilla.org/r/81256/#review88678

One more suggestion to simplify the implementation, and some tiny nits.

::: browser/components/extensions/ext-omnibox.js:94
(Diff revisions 16 - 17)
>  
>        onInputChangedInternal: new SingletonEventManager(context, "omnibox.onInputChangedInternal", fire => {
> -        let listener = (eventName, searchString, suggestionsCallback) => {
> -          let keyword = gKeywordMap.get(extension);
> +        let listener = (eventName, searchString, id, callback) => {
> +          gSuggestionsCallbackMap.set(extension, {id, callback});
> -          gSuggestionsCallbackMap.set(keyword, suggestionsCallback);
>            fire(searchString);

If you make the suggestion callback a method of `ExtensionSearchHandler` (as suggested at your other patch - https://bugzil.la/1267810#c85), and pass the `id` to the event listener here, via `fire(id, searchString);`
then `gSuggestionsCallbackMap` is not needed any more (so there are less concerns about the lifetime of the callback).

In the child, the ID can be passed up back again.
```
let listener = (id, searchString) => {
  runSafeSyncWithoutClone(fire, searchString, suggestions => {
    context.childManager.callParentAsyncFunction("omnibox.addSuggestionsInternal", [
      id,
      suggestions,
    ]);
  });
};
```

And `addSuggestionsInternal` can then call that new method on ExtensionSearchHandler to complete the deal.

::: browser/components/extensions/schemas/omnibox.json:23
(Diff revisions 16 - 17)
>                      "type": "string",
>                      "pattern": "^[^?\\s:][^\\s:]*[^/\\s:]$"
>                    },
>                    {
>                      "type": "string",
> -                    "pattern": "^[^?\\s:/*]$"
> +                    "pattern": "^[^?\\s:/]$"

Since there is no reason for disallowing an asterisk, I guess that the forbidden slash is also arbitrary?

If so then I suggest to allow the slash too and combine the two patterns:

`"^[^?\\s:]([^\\s:]*[^/\\s:])?$"`

::: browser/components/extensions/test/browser/browser_ext_omnibox.js:228
(Diff revisions 16 - 17)
> +      extension.sendMessage(info.test)
> +      yield extension.awaitMessage("test-ready");
> +      text = yield startInputSession();
> +    } else {
> +      text = yield startInputSession();
> +      extension.sendMessage(info.test)

Nit: Missing semicolon.

::: browser/components/extensions/test/browser/browser_ext_omnibox.js:280
(Diff revisions 16 - 17)
> +  ];
> +
> +  // Test adding suggesions asynchronously. Order here matters.
> +  yield testAsynchronousSuggestions({
> +    test: "test-multiple-suggest-calls",
> +    suggestions

Nit: Missing trailing comma (also up and below).
Comment on attachment 8795085 [details]
Bug 1267810 - Use the new module to implement the omnibox api.

https://reviewboard.mozilla.org/r/81256/#review88718

(clearing review flag again. In terms of functionality and test coverage it is r+, but I'd like to see the change above before it's committed, so r-).
Attachment #8795085 - Flags: review?(rob) → review-
Comment on attachment 8795084 [details]
Bug 1267810 - Add a module for registering keywords and handling keyword input sessions.

https://reviewboard.mozilla.org/r/81254/#review89068

I'm not familiar with the urlbar binding logic, but it looks reasonable to me.

::: browser/base/content/urlbarBindings.xml:625
(Diff revisions 15 - 16)
>                Services.search.getEngineByName(engineOrEngineName) :
>                engineOrEngineName;
> -          BrowserSearch.recordSearchInTelemetry(engine, "urlbar");
> +          let isOneOff = this.popup.oneOffSearchButtons
> -          this.popup.oneOffSearchButtons
>                .maybeRecordTelemetry(event, openUILinkWhere, openUILinkParams);
> +          // Infer the type of the even which triggered the search.

Nit: s/even/event/

::: browser/base/content/urlbarBindings.xml:1414
(Diff revisions 15 - 16)
>                  onset="this._overrideValue = val; return val;"/>
>  
> +      <method name="onPopupClick">
> +        <parameter name="aEvent"/>
> +        <body><![CDATA[
> +          if (aEvent.button == 2) {

Do you need to worry about key modifiers? altKey / shiftKey / ctrlKey / metaKey

::: toolkit/components/places/ExtensionSearchHandler.jsm:110
(Diff revisions 15 - 16)
>    /**
> +   * Adds suggestions for the registered keyword. The callback ID is provided
> +   * by handleSearch
> +   *
> +   * @param {string} keyword The keyword.
> +   * @param {string} suggestion The text to use for the heuristic result.

Nit: JSDoc is missing a `@param` for the callbackID.

::: toolkit/components/places/ExtensionSearchHandler.jsm:139
(Diff revisions 15 - 16)
> -   * provide suggestions to the navigation bar. The callback is invalidated when
> -   * either the input changes or the urlbar blurs.
> +   * used to provide suggestions to the urlbar while the callback ID is active.
> +   * The callback is invalidated when either the input changes or the urlbar blurs.
>     *
>     * @param {string} keyword The keyword to handle.
>     * @param {string} searchString The search text in the location bar.
>     * @param {string} callback The callback used to provide search suggestions.

Is this method optional? If not, then you might want to add a callback to `test_correct_events_are_emitted`.

(And I expected a test failure, because addSuggestions unconditionally calls `gSuggestionsCallback`.)

::: toolkit/components/places/ExtensionSearchHandler.jsm:150
(Diff revisions 15 - 16)
>  
> -    let keywordInfo = gKeywordMap.get(keyword);
> -    let extension = keywordInfo.extension;
> +    if (gActiveKeyword && keyword != gActiveKeyword) {
> +      throw new Error("A different input session is already ongoing");
> +    }
> +
> +    let {extension} = gKeywordMap.get(keyword);

Nit: There are other places in this file where you can use `let {extension} = gKeywordMap.get(keyword);` for consistency.

::: toolkit/components/places/ExtensionSearchHandler.jsm:155
(Diff revisions 15 - 16)
> +    let {extension} = gKeywordMap.get(keyword);
>  
>      // Only emit MSG_INPUT_STARTED when the session first becomes active.
> -    // This is slightly different from Chrome's behavior (which fires
> -    // MSG_INPUT_STARTED when MSG_INPUT_CHANGED first fires, but according to
> -    // https://bugs.chromium.org/p/chromium/issues/detail?id=258911 that
> +    // This is different from Chrome's behavior, which fires MSG_INPUT_STARTED
> +    // when MSG_INPUT_CHANGED first fires, but this is a bug in Chrome according
> +    // to https://bugs.chromium.org/p/chromium/issues/detail?id=258911.

FYI you can use https://crbug.com/258911 instead of the long URL.
Comment on attachment 8795085 [details]
Bug 1267810 - Use the new module to implement the omnibox api.

https://reviewboard.mozilla.org/r/81256/#review89074

Nice :)

::: browser/components/extensions/ext-omnibox.js:85
(Diff revisions 17 - 18)
>          };
>        }).api(),
> +    },
>  
> +    omnibox_internal: {
>        onInputChangedInternal: new SingletonEventManager(context, "omnibox.onInputChangedInternal", fire => {

`omnibox_internal.onInputChangedInternal` - One of the "internal" can be removed to make the name shorter.

I slightly favor using just `omnibox.*Internal` though, because then it is immediately obvious that the method is internal, by just looking at the method name. But I'll let it be your call.

::: browser/components/extensions/schemas/omnibox.json:204
(Diff revisions 17 - 18)
>      ]
> +  },
> +  {
> +    "namespace": "omnibox_internal",
> +    "description": "The internal namespace used by the omnibox API.",
> +    "allowedContexts": ["addon_parent_only"],

If you add `"defaultContexts": ["addon_parent_only"],` then it is used in the functions under this namespace, unless another `"allowedContexts"` is declared.
Attachment #8795085 - Flags: review?(rob) → review+
Comment on attachment 8795084 [details]
Bug 1267810 - Add a module for registering keywords and handling keyword input sessions.

https://reviewboard.mozilla.org/r/81254/#review89068

> Nit: s/even/event/

Done.

> Do you need to worry about key modifiers? altKey / shiftKey / ctrlKey / metaKey

I do when a suggestion is clicked on (in order to determine the disposition for `onInputEntered`), but this information is already provided in the `where` argument I pass to `ExtensionSearchHandler.handleInputEntered`.

> Nit: JSDoc is missing a `@param` for the callbackID.

Oops, good catch, I've updated this JSDoc.

> Is this method optional? If not, then you might want to add a callback to `test_correct_events_are_emitted`.
> 
> (And I expected a test failure, because addSuggestions unconditionally calls `gSuggestionsCallback`.)

The callback should not be optional, but this test won't throw an error because the mock extension used never calls `ExtensionSearchHandler.addSuggestions`. I'll update this function to throw an error though if the callback is missing and add a test to `test_correct_errors_are_thrown`.

> Nit: There are other places in this file where you can use `let {extension} = gKeywordMap.get(keyword);` for consistency.

Done.

> FYI you can use https://crbug.com/258911 instead of the long URL.

Thanks :)
Comment on attachment 8795085 [details]
Bug 1267810 - Use the new module to implement the omnibox api.

https://reviewboard.mozilla.org/r/81256/#review89074

Thanks for the reviews!

> `omnibox_internal.onInputChangedInternal` - One of the "internal" can be removed to make the name shorter.
> 
> I slightly favor using just `omnibox.*Internal` though, because then it is immediately obvious that the method is internal, by just looking at the method name. But I'll let it be your call.

I think using a separate namespace is nicer because it groups all of the internal methods together and makes it more clear which functions are going to be exposed in the `omnibox` namespace. Also, we provide the namespace when calling `callParentAsyncFunction` so I think that also helps.

> If you add `"defaultContexts": ["addon_parent_only"],` then it is used in the functions under this namespace, unless another `"allowedContexts"` is declared.

Nice :)
Comment on attachment 8795085 [details]
Bug 1267810 - Use the new module to implement the omnibox api.

https://reviewboard.mozilla.org/r/81256/#review89718

All little nits at this point

::: browser/components/extensions/ext-omnibox.js:30
(Diff revision 19)
> +  }
> +});
> +
> +extensions.on("shutdown", (type, extension) => {
> +  let keyword = gKeywordMap.get(extension);
> +  // The keyword will be undefined if the extension tried to register

This comment is misleading, the keyword will also be undefined if the extension didn't try to register a keyword.  I'm not sure you need a comment at all.

::: browser/components/extensions/ext-omnibox.js:54
(Diff revision 19)
> +          return Promise.reject(e.message);
> +        }
> +      },
> +
> +      onInputStarted: new SingletonEventManager(context, "omnibox.onInputStarted", fire => {
> +        let listener = () => {

listsner is identical to fire, no need to wrap it
(same below as well)

::: browser/components/extensions/test/browser/browser_ext_omnibox.js:62
(Diff revision 19)
> +            browser.test.sendMessage("test-ready");
> +            break;
> +          case "test-suggestions-with-delays":
> +            let delay = 1;
> +            suggestions.forEach(suggestion =>
> +              setTimeout(() => suggestCallback([suggestion]), delay++));

You're having the delay increase between each successive call, is that intentional?  I think you could avoid setTimeout() altogether and just use Promise.resolve().then(...) to defer each callback

::: browser/components/extensions/test/browser/browser_ext_omnibox.js:71
(Diff revision 19)
> +      });
> +    },
> +  });
> +
> +  function* expectEvent(event, expected = {}) {
> +    info(`Waiting for event "${event}" to fire`);

Can you take out the debugging statements before landing

::: browser/components/extensions/test/browser/browser_ext_omnibox.js:201
(Diff revision 19)
> +    });
> +  }
> +
> +  function* testAsynchronousSuggestions(info) {
> +    extension.sendMessage("set-synchronous", {synchronous: false});
> +    yield extension.awaitMessage("synchronous-set");

I don't think you need to wait here, your next action is to send another message to the extension and those messages will be received and processed in order.

::: browser/components/extensions/test/browser/browser_ext_omnibox.js:206
(Diff revision 19)
> +    yield extension.awaitMessage("synchronous-set");
> +
> +    function expectSuggestion({content, description}, index) {
> +      let item = gURLBar.popup.richlistbox.getItemAtIndex(index + 1);
> +
> +      is(item.getAttribute("title"), `${description}`,

No need for the quotes for the second argument

::: browser/components/extensions/test/browser/browser_ext_omnibox.js:297
(Diff revision 19)
> +        "keyword": keyword,
> +      },
> +    },
> +  });
> +
> +  SimpleTest.waitForExplicitFinish();

why is this needed?
Attachment #8795085 - Flags: review?(aswan) → review+
No longer depends on: 1309047
Flags: needinfo?(mwein)
Comment on attachment 8795085 [details]
Bug 1267810 - Use the new module to implement the omnibox api.

https://reviewboard.mozilla.org/r/81256/#review87656

> If the console does not show meaningful errors (I didn't test), it wouldn't hurt to validate the type of suggestions here, to help developers with debugging.

Schema.jsm already provides reasonable type validation for this.

> `registerKeyword` can throw if the keyword was already known to `ExtensionSearchHandler`, which prevents the extension from loading. This is a big thing.
> 
> I think that it's better to gracefully ignore the registration and print an error and/or reject invocations to methods such as setDefaultSuggestion if the keyword is unavailable.

Done.

> This is the default return value, there is no need for `return Promise.resolve();`

I didn't know that, thanks.

Removed.

> See my comments below in `omnibox.onInputChangedInternal`.
> 
> You should pass the suggestion callback ID here, and use that ID to fetch the relevant callback from the global map.
> 
> The reason for using the ID here is to make sure that callbacks for old onInputChanged calls are ignored.
> 
> Alternatively, you can stick to the use of the keyword, but then you have to make sure that ExtensionSearchHandler ignores callbacks for old queries.

Sounds good, I've added support for using a callback ID in the next revision.

> See my review at `_matchExtensionSuggestions` in your other patch.
> 
> You should pass down a unique value (e.g. a counter), and use this as the key for the map.
> 
> I will refer to this unique value as "suggestion callback ID" in the rest of my review, for clarity.

Done.

> Do not forget to clean up the `gSuggestionsCallbackMap` here. Presently, when an extension page reloads, the callback will still be lingering in the global map.
> 
> To clean up, it suffices to delete the callback with the suggestion callback ID. This ID is only available in the "listener" callback, but you can create a variable in the same scope as "listener" to store *the* last known ID. There should be only one, because presumably there is only one omnibox/awesomebar.
> 
> Following that assumption, you should be able to delete the pending suggestion callback (if any) before setting a new one in the listener above.

Done.

> It seems that this test event is used in `startInput*`, and that its callers expect a search suggestion to be ready. To avoid any timing issues (e.g. if somehow IPC is so busy that the main process receives and processes the `test.sendMessage` call before `suggest` is called), move the `test.sendMessage` call after `suggest`.

Done.

> Please add a comment that explains the meaning of this arbitrary number.

This number was related to the timing issue you noticed in the previous comment, so the delay is no longer needed. Thanks!

> Please add tests for the following scenarios:
> 
> - suggest is called asynchronously.
> - suggest is called twice (expected: the results stack up).
> - suggest is called twice with a delay between the two calls (same expectation).
> - suggest is called after the user has triggered more input (expected: the previous call is ignored).

Done.

> Why is a single asterisk not allowed? The questionmark marks a search query, but AFAIK there is nothing special with asterisks.

It wasn't working when I tested it, but on second thought, it's probably better not to exclude it here.

> s/mult/multi/

Fixed.

> s/miscelaneous/miscellaneous/

Fixed.
Comment on attachment 8795085 [details]
Bug 1267810 - Use the new module to implement the omnibox api.

https://reviewboard.mozilla.org/r/81256/#review88678

> If you make the suggestion callback a method of `ExtensionSearchHandler` (as suggested at your other patch - https://bugzil.la/1267810#c85), and pass the `id` to the event listener here, via `fire(id, searchString);`
> then `gSuggestionsCallbackMap` is not needed any more (so there are less concerns about the lifetime of the callback).
> 
> In the child, the ID can be passed up back again.
> ```
> let listener = (id, searchString) => {
>   runSafeSyncWithoutClone(fire, searchString, suggestions => {
>     context.childManager.callParentAsyncFunction("omnibox.addSuggestionsInternal", [
>       id,
>       suggestions,
>     ]);
>   });
> };
> ```
> 
> And `addSuggestionsInternal` can then call that new method on ExtensionSearchHandler to complete the deal.

Yeah, this sounds great, I'll update the patch to do this.

> Since there is no reason for disallowing an asterisk, I guess that the forbidden slash is also arbitrary?
> 
> If so then I suggest to allow the slash too and combine the two patterns:
> 
> `"^[^?\\s:]([^\\s:]*[^/\\s:])?$"`

Sgtm, thanks.

> Nit: Missing semicolon.

Fixed.

> Nit: Missing trailing comma (also up and below).

Fixed.
Comment on attachment 8795085 [details]
Bug 1267810 - Use the new module to implement the omnibox api.

https://reviewboard.mozilla.org/r/81256/#review89718

Thanks for the review!

> This comment is misleading, the keyword will also be undefined if the extension didn't try to register a keyword.  I'm not sure you need a comment at all.

Oh that makes sense, I'll remove the comment.

> listsner is identical to fire, no need to wrap it
> (same below as well)

It's actually slightly different because it fires with the name of the event as the first arguement.

I changed it to the following to make it more clear:
```
        let listener = (eventName) => {
          fire();
        };
```

> You're having the delay increase between each successive call, is that intentional?  I think you could avoid setTimeout() altogether and just use Promise.resolve().then(...) to defer each callback

It was intential but it's also a bit overkill. Using Promise.resolve().then(...) is cleaner and better. Thanks for the suggestion.

> I don't think you need to wait here, your next action is to send another message to the extension and those messages will be received and processed in order.

Removed.

> why is this needed?

I guess it's not really needed, but if it's removed the following error is logged at the end of the test run:

```
The following tests failed:
TEST-UNEXPECTED-FAIL | unknown test url | Console monitoring requires use of waitForExplicitFinish.
```
Keywords: checkin-needed
Comment on attachment 8795085 [details]
Bug 1267810 - Use the new module to implement the omnibox api.

https://reviewboard.mozilla.org/r/81256/#review89878

::: browser/components/extensions/ext-c-omnibox.js:18
(Diff revision 20)
> +  return {
> +    omnibox: {
> +      onInputChanged: new SingletonEventManager(context, "omnibox.onInputChanged", fire => {
> +        let listener = (text, id) => {
> +          runSafeSyncWithoutClone(fire, text, suggestions => {
> +            context.childManager.callParentAsyncFunction("omnibox_internal.addSuggestions", [

I just noticed - this method does not use any async behavior.

Can you change this to `callParentFunctionNoReturn` instead, and remove the async declaration from the manifest? That would be slightly more efficient than the current implementation.
Comment on attachment 8795085 [details]
Bug 1267810 - Use the new module to implement the omnibox api.

https://reviewboard.mozilla.org/r/81256/#review89878

> I just noticed - this method does not use any async behavior.
> 
> Can you change this to `callParentFunctionNoReturn` instead, and remove the async declaration from the manifest? That would be slightly more efficient than the current implementation.

I updated this to use `callParentFunctionNoReturn`, but if I remove the async declaration from the manifest I get `TypeError: deferred is undefined` at line 1884 in ExtensionUtils.jsm.
Comment on attachment 8795085 [details]
Bug 1267810 - Use the new module to implement the omnibox api.

https://reviewboard.mozilla.org/r/81256/#review89878

> I updated this to use `callParentFunctionNoReturn`, but if I remove the async declaration from the manifest I get `TypeError: deferred is undefined` at line 1884 in ExtensionUtils.jsm.

Did you also remove the unused/non-existent function parameter from the schema?
Comment on attachment 8795085 [details]
Bug 1267810 - Use the new module to implement the omnibox api.

https://reviewboard.mozilla.org/r/81256/#review89878

> Did you also remove the unused/non-existent function parameter from the schema?

Yeah I did
 conflicts while merging toolkit/components/places/UnifiedComplete.js! (edit, then use 'hg resolve --mark') unresolved conflicts (see hg resolve, then hg rebase --continue)
Flags: needinfo?(mwein)
Keywords: checkin-needed
Comment on attachment 8795085 [details]
Bug 1267810 - Use the new module to implement the omnibox api.

https://reviewboard.mozilla.org/r/81256/#review89878

> Yeah I did

This error seems to occur when an error is triggered, I have filed a bug for that: 1314903. The TypeError error probably appears when the `omnibox_internal.addSuggestions` method throws. The method calls `ExtensionSearchHandler.addSuggestions`, which throws an error when the callback is redundant. Should we really print such an error? Extension authors cannot prevent this from happening because they don't know when the user is going to type. So I suggest to wrap that call in a try-catch to avoid errors.

Or (since the error values are not used anywhere), return a boolean from addSuggestions (and use the booleans in the unit tests instead of assertThrows)
Comment on attachment 8795085 [details]
Bug 1267810 - Use the new module to implement the omnibox api.

https://reviewboard.mozilla.org/r/81256/#review89878

> This error seems to occur when an error is triggered, I have filed a bug for that: 1314903. The TypeError error probably appears when the `omnibox_internal.addSuggestions` method throws. The method calls `ExtensionSearchHandler.addSuggestions`, which throws an error when the callback is redundant. Should we really print such an error? Extension authors cannot prevent this from happening because they don't know when the user is going to type. So I suggest to wrap that call in a try-catch to avoid errors.
> 
> Or (since the error values are not used anywhere), return a boolean from addSuggestions (and use the booleans in the unit tests instead of assertThrows)

Thanks for filing bug 1314903. You're right that we shouldn't throw that error because of the case you described. I switched to wrapping `ExtensionSearchHandler.addSuggestions` with a try-catch to fix this. However, I'm going to change `callParentFunctionNoReturn` back to `callParentAsyncFunction` for now because the function can also throw if the suggestions array is malformed. I've added a TODO to change this once bug 1314903 is fixed.
Flags: needinfo?(mwein)
Keywords: checkin-needed
Pushed by ihsiao@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/5b9b56fecb95
Add a module for registering keywords and handling keyword input sessions. r=adw
https://hg.mozilla.org/integration/autoland/rev/be4455a7669f
Use the new module to implement the omnibox api. r=aswan,robwu
Keywords: checkin-needed
Sorry had to back out this for eslint failure, https://treeherder.mozilla.org/logviewer.html#?job_id=6128648&repo=autoland#L5438
Flags: needinfo?(mwein)
Backout by ihsiao@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/8303f902a05e
Backed out changeset be4455a7669f for eslint failure
https://hg.mozilla.org/integration/autoland/rev/f95a98c9eeaf
Backed out changeset 5b9b56fecb95
Much naughtier than just lint, on debug asserting, https://treeherder.mozilla.org/logviewer.html#?job_id=6130339&repo=autoland, and on opt going as far as trying to connect to google.com, which is a fatal error, https://treeherder.mozilla.org/logviewer.html#?job_id=6130082&repo=autoland
Fixed the eslint error. Now running Try to see if the debug assertion is fixed: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2806cfd98fbd11eafb0b2e044f4991433e4f38bd
Flags: needinfo?(mwein)
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/9cfc41a2869e
Add a module for registering keywords and handling keyword input sessions. r=adw
https://hg.mozilla.org/integration/autoland/rev/7ef35cccfd7b
Use the new module to implement the omnibox api. r=aswan,robwu
Keywords: checkin-needed
Backout by cbook@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0b58909e7626
Backed out changeset 7ef35cccfd7b 
https://hg.mozilla.org/integration/autoland/rev/4769fe1cc004
Backed out changeset 9cfc41a2869e for eslint failures
I fixed the eslint error. This is a new eslint rule which hasn't merged into central yet so that's why my local eslint didn't catch it.
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/02cecde2f137
Add a module for registering keywords and handling keyword input sessions. r=adw
https://hg.mozilla.org/integration/autoland/rev/007b565539fa
Use the new module to implement the omnibox api. r=aswan,robwu
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/02cecde2f137
https://hg.mozilla.org/mozilla-central/rev/007b565539fa
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
(In reply to Daniel Veditz [:dveditz] from comment #57)
> Tossing this needinfo? to Tanvi since there's overlap with her work on the
> Site Identity box.

Clearing my needinfo.  My work has mostly been with the Site Identity box and Control Center itself.  I don't really have an opinion on this.
Flags: needinfo?(tanvi)
Depends on: 1318910
Keywords: dev-doc-needed
I was just trying out the new omnibox API and found that the "description" field of a suggestion is just plain text.
In Chrome it is treated as XML, allowing add-ons to have slightly more expressiveness in showing a suggestion.
It is documented at https://developer.chrome.com/extensions/omnibox#type-SuggestResult

<url> - green, visually emphasized as a URL
<match> - boldfaced emphasized text
<dim> - grey, de-emphasized text
XML entities such as &gt;, &lt, &amp; decode to <, >, &

Are there plans to support this XML syntax too? If not then the incompatibility should be documented on MDN.

Here is an example of such an extension: https://chrome.google.com/webstore/detail/imdb-search/cbongpcdgehbfeajgkndgkbdkkfdmdik
Flags: needinfo?(mwein)
Yeah, I created a tracking bug to add support for this - bug 1323091.
Flags: needinfo?(mwein)
I've written some docs for this API: https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/omnibox

Please let me know if they look all right.
Flags: needinfo?(mwein)
This looks great, thanks!
Flags: needinfo?(mwein)
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.