Make chrome://omnibox/ be able to report per-provider matches.

Make chrome://omnibox/ be able to report per-provider matches.

This will be very useful for those of us working
on improving the scoring/ranking of autocomplete
suggestions.

The presentation method could probably be improved, but I'd prefer
to check this in as-is, and see how us omnibox developers use it in
practice.  Then we'll have a better idea of how chrome://omnibox/
should convey the information we most need.

BUG=111791
TEST=


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=120090

[Mark P, 2012-01-25 23:21:07]

Hey reviewers,

Here's how to divide up the review:

arv - the .js and .html files

Evan - the omnibox_ui_handler.{cc,h}

Peter - the two lines in autocomplete.h

thanks,
mark

https://chromiumcodereview.appspot.com/9289028/diff/1/chrome/browser/resource...
File chrome/browser/resources/omnibox/omnibox.js (left):

https://chromiumcodereview.appspot.com/9289028/diff/1/chrome/browser/resource...
chrome/browser/resources/omnibox/omnibox.js:92: '== default_match iterator', '',
'is_default_match', false),
I removed is_default_match because we as developers found it unnecessary.

https://chromiumcodereview.appspot.com/9289028/diff/1/chrome/browser/ui/webui...
File chrome/browser/ui/webui/omnibox/omnibox_ui_handler.cc (left):

https://chromiumcodereview.appspot.com/9289028/diff/1/chrome/browser/ui/webui...
chrome/browser/ui/webui/omnibox/omnibox_ui_handler.cc:103:
result_to_output.SetBoolean(prefix + ".is_default_match",
I removed is_default_match because keeping it would complicate the code and we
as developers found it unnecessary.

[Peter Kasting, 2012-01-26 00:19:29]

LGTM

https://chromiumcodereview.appspot.com/9289028/diff/1/chrome/browser/autocomp...
File chrome/browser/autocomplete/autocomplete.h (right):

https://chromiumcodereview.appspot.com/9289028/diff/1/chrome/browser/autocomp...
chrome/browser/autocomplete/autocomplete.h:739: friend class OmniboxUIHandler; 
// to provide access to providers_
Nit: List friends atop the private: section.

It might be better in this case to go ahead and expose a getter.  I'm not
obsessed with keeping |providers_| secret.

[Evan Stade, 2012-01-26 00:32:49]

lgtm

[Evan Stade, 2012-01-26 00:32:56]

https://chromiumcodereview.appspot.com/9289028/diff/1/chrome/browser/ui/webui...
File chrome/browser/ui/webui/omnibox/omnibox_ui_handler.cc (right):

https://chromiumcodereview.appspot.com/9289028/diff/1/chrome/browser/ui/webui...
chrome/browser/ui/webui/omnibox/omnibox_ui_handler.cc:76: it !=
controller_->providers_.end(); ++it)
curlies

[Mark P, 2012-01-26 22:11:59]

http://codereview.chromium.org/9289028/diff/1/chrome/browser/autocomplete/aut...
File chrome/browser/autocomplete/autocomplete.h (right):

http://codereview.chromium.org/9289028/diff/1/chrome/browser/autocomplete/aut...
chrome/browser/autocomplete/autocomplete.h:739: friend class OmniboxUIHandler; 
// to provide access to providers_
On 2012/01/26 00:19:29, Peter Kasting wrote:
> Nit: List friends atop the private: section.
> 
> It might be better in this case to go ahead and expose a getter.  I'm not
> obsessed with keeping |providers_| secret.

Okay, created a providers() getter; removed the friend declaration.

I made const ACProviders*.  (I don't want it to return a const ACProviders
itself because that would make a copy.)  If you want me to make it return a
const ACProviders& instead, I can easily do that.

http://codereview.chromium.org/9289028/diff/1/chrome/browser/ui/webui/omnibox...
File chrome/browser/ui/webui/omnibox/omnibox_ui_handler.cc (right):

http://codereview.chromium.org/9289028/diff/1/chrome/browser/ui/webui/omnibox...
chrome/browser/ui/webui/omnibox/omnibox_ui_handler.cc:76: it !=
controller_->providers_.end(); ++it)
On 2012/01/26 00:32:56, Evan Stade wrote:
> curlies

Done.

[Mark P, 2012-01-31 16:56:49]

Arv,

Do you have an ETA on when you'll get to look at this changelist?

thanks,
mark


On 2012/01/25 23:21:07, Mark P wrote:
> Hey reviewers,
> 
> Here's how to divide up the review:
> 
> arv - the .js and .html files
> 
> Evan - the omnibox_ui_handler.{cc,h}
> 
> Peter - the two lines in autocomplete.h
> 
> thanks,
> mark
> 
>
https://chromiumcodereview.appspot.com/9289028/diff/1/chrome/browser/resource...
> File chrome/browser/resources/omnibox/omnibox.js (left):
> 
>
https://chromiumcodereview.appspot.com/9289028/diff/1/chrome/browser/resource...
> chrome/browser/resources/omnibox/omnibox.js:92: '== default_match iterator',
'',
> 'is_default_match', false),
> I removed is_default_match because we as developers found it unnecessary.
> 
>
https://chromiumcodereview.appspot.com/9289028/diff/1/chrome/browser/ui/webui...
> File chrome/browser/ui/webui/omnibox/omnibox_ui_handler.cc (left):
> 
>
https://chromiumcodereview.appspot.com/9289028/diff/1/chrome/browser/ui/webui...
> chrome/browser/ui/webui/omnibox/omnibox_ui_handler.cc:103:
> result_to_output.SetBoolean(prefix + ".is_default_match",
> I removed is_default_match because keeping it would complicate the code and we
> as developers found it unnecessary.

[arv (Not doing code reviews), 2012-01-31 18:55:36]

https://chromiumcodereview.appspot.com/9289028/diff/6001/chrome/browser/resou...
File chrome/browser/resources/omnibox/omnibox.js (right):

https://chromiumcodereview.appspot.com/9289028/diff/6001/chrome/browser/resou...
chrome/browser/resources/omnibox/omnibox.js:28:
document.getElementById('show-details').addEventListener(
How about listing to an ancestor?

$('omnibox-input-form').addEventListener('change', refresh, true);

'change' events do not bubble but all events propagate in the capture phase.

https://chromiumcodereview.appspot.com/9289028/diff/6001/chrome/browser/resou...
chrome/browser/resources/omnibox/omnibox.js:196: *       { 'destination_url':
'http://mail.google.com',
{
  'key': value
  ...
}

https://chromiumcodereview.appspot.com/9289028/diff/6001/chrome/browser/resou...
chrome/browser/resources/omnibox/omnibox.js:251:
p.appendChild(document.createTextNode('combined results:'));
textContent

https://chromiumcodereview.appspot.com/9289028/diff/6001/chrome/browser/resou...
chrome/browser/resources/omnibox/omnibox.js:266: * @param {Object} |result| is
either the combined_results component
@param {Type} paramName Description.

[Mark P, 2012-01-31 20:10:40]

https://chromiumcodereview.appspot.com/9289028/diff/6001/chrome/browser/resou...
File chrome/browser/resources/omnibox/omnibox.js (right):

https://chromiumcodereview.appspot.com/9289028/diff/6001/chrome/browser/resou...
chrome/browser/resources/omnibox/omnibox.js:28:
document.getElementById('show-details').addEventListener(
On 2012/01/31 18:55:36, arv wrote:
> How about listing to an ancestor?

Sure.  Done.  See corresponding HTML changes (if you care).

> 
> $('omnibox-input-form').addEventListener('change', refresh, true);
> 
> 'change' events do not bubble but all events propagate in the capture phase.

https://chromiumcodereview.appspot.com/9289028/diff/6001/chrome/browser/resou...
chrome/browser/resources/omnibox/omnibox.js:196: *       { 'destination_url':
'http://mail.google.com',
On 2012/01/31 18:55:36, arv wrote:
> {
>   'key': value
>   ...
> }

I was wondering if someone who call me on the inconsistent formatting.  I snuck
by Evan--I guess he didn't care--but you said something.  Fixed it, and fixed
the corresponding comment in the C++ code.

https://chromiumcodereview.appspot.com/9289028/diff/6001/chrome/browser/resou...
chrome/browser/resources/omnibox/omnibox.js:251:
p.appendChild(document.createTextNode('combined results:'));
On 2012/01/31 18:55:36, arv wrote:
> textContent

Done.

https://chromiumcodereview.appspot.com/9289028/diff/6001/chrome/browser/resou...
chrome/browser/resources/omnibox/omnibox.js:266: * @param {Object} |result| is
either the combined_results component
On 2012/01/31 18:55:36, arv wrote:
> @param {Type} paramName Description.

Fixed.

[arv (Not doing code reviews), 2012-01-31 20:29:05]

https://chromiumcodereview.appspot.com/9289028/diff/12001/chrome/browser/reso...
File chrome/browser/resources/omnibox/omnibox.html (right):

https://chromiumcodereview.appspot.com/9289028/diff/12001/chrome/browser/reso...
chrome/browser/resources/omnibox/omnibox.html:18: <div
id="omnibox-input-form-options">
This is kind of ugly. I'm sorry I lead you down this path

https://chromiumcodereview.appspot.com/9289028/diff/12001/chrome/browser/reso...
File chrome/browser/resources/omnibox/omnibox.js (right):

https://chromiumcodereview.appspot.com/9289028/diff/12001/chrome/browser/reso...
chrome/browser/resources/omnibox/omnibox.js:29: 'change', refresh);
you need to pass a third argument here

[Mark P, 2012-01-31 20:55:34]

https://chromiumcodereview.appspot.com/9289028/diff/12001/chrome/browser/reso...
File chrome/browser/resources/omnibox/omnibox.html (right):

https://chromiumcodereview.appspot.com/9289028/diff/12001/chrome/browser/reso...
chrome/browser/resources/omnibox/omnibox.html:18: <div
id="omnibox-input-form-options">
On 2012/01/31 20:29:05, arv wrote:
> This is kind of ugly. I'm sorry I lead you down this path

Do you want me to go back to registering three listeners?

I don't care much either way.

https://chromiumcodereview.appspot.com/9289028/diff/12001/chrome/browser/reso...
File chrome/browser/resources/omnibox/omnibox.js (right):

https://chromiumcodereview.appspot.com/9289028/diff/12001/chrome/browser/reso...
chrome/browser/resources/omnibox/omnibox.js:29: 'change', refresh);
On 2012/01/31 20:29:05, arv wrote:
> you need to pass a third argument here

false?

You do realize that in an earlier code review where I had false you told me to
remove it.
http://codereview.chromium.org/9129029/diff/1/chrome/browser/resources/omnibo...

[arv (Not doing code reviews), 2012-01-31 20:59:35]

https://chromiumcodereview.appspot.com/9289028/diff/12001/chrome/browser/reso...
File chrome/browser/resources/omnibox/omnibox.html (right):

https://chromiumcodereview.appspot.com/9289028/diff/12001/chrome/browser/reso...
chrome/browser/resources/omnibox/omnibox.html:18: <div
id="omnibox-input-form-options">
On 2012/01/31 20:55:34, Mark P wrote:
> On 2012/01/31 20:29:05, arv wrote:
> > This is kind of ugly. I'm sorry I lead you down this path
> 
> Do you want me to go back to registering three listeners?
> 
> I don't care much either way.

Yeah, js should adjust to dom, not the other way around.

[Mark P, 2012-01-31 21:14:43]

https://chromiumcodereview.appspot.com/9289028/diff/12001/chrome/browser/reso...
File chrome/browser/resources/omnibox/omnibox.html (right):

https://chromiumcodereview.appspot.com/9289028/diff/12001/chrome/browser/reso...
chrome/browser/resources/omnibox/omnibox.html:18: <div
id="omnibox-input-form-options">
On 2012/01/31 20:59:35, arv wrote:
> On 2012/01/31 20:55:34, Mark P wrote:
> > On 2012/01/31 20:29:05, arv wrote:
> > > This is kind of ugly. I'm sorry I lead you down this path
> > 
> > Do you want me to go back to registering three listeners?
> > 
> > I don't care much either way.
> 
> Yeah, js should adjust to dom, not the other way around.

Switched back.

[arv (Not doing code reviews), 2012-01-31 23:30:27]

LGTM

[commit-bot: I haz the power, 2012-02-01 15:40:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mpearson@chromium.org/9289028/16003

[commit-bot: I haz the power, 2012-02-01 16:14:36]

Try job failure for 9289028-16003 on linux_rel for step "update".
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

Step "update" is always a major failure.
Look at the try server FAQ for more details.

[Mark P, 2012-02-01 18:35:53]

On 2012/02/01 16:14:36, I haz the power (commit-bot) wrote:
> Try job failure for 9289028-16003 on linux_rel for step "update".
>
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...
> 
> Step "update" is always a major failure.

Update failed due to lack of disk space on the bot.  Re-trying.

[commit-bot: I haz the power, 2012-02-01 18:36:19]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mpearson@chromium.org/9289028/16003

[commit-bot: I haz the power, 2012-02-01 20:07:09]

Change committed as 120090