Enhance chrome://omnibox Presentation

Enhance chrome://omnibox Presentation

Added ability of providers to append arbitrary information to be presented in the omnibox metrics window.
Adjust formatting somewhat.

BUG=None
TEST=Observe
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=148905

[mrossetti, 2012-07-03 18:16:47]

Please look this over and assure me that I am heading in the right direction.

I am not satisfied with the table layout: I tried to restrict the width of the
'URL' column and failed. Perhaps you could suggest the proper fix. (I tried
setting max-width, using a custom class, etc.) I think the 'URL' column being to
wide prevented the 'Logged Info' column from widening properly.

[Mark P, 2012-07-03 19:06:14]

On 2012/07/03 18:16:47, mrossetti wrote:
> Please look this over and assure me that I am heading in the right direction.

I'm sorry: I can't assure you that you're heading in the right direction.  :)

I thought this change was to add additional useful information the
chrome://omnibox window.  If so, why are you messing with logs?

I have some concerns about this change as is:
* If you want to add something to the logs, we need to talk to the privacy
people *first*.
* If you want to add something to the logs, this is the wrong way to do.  Chrome
has a policy of not using raw strings in logs.  We use hashes of strings or
enums.  Talk to me for details if you still want to log things.
* If you want to add a repeated dictionary-like object to the logs, you're going
to get (quite reasonable) pushback about the design.  I know I did when I wanted
to add per-provider logging.
* You should not remove fields from protocol buffers.  If they're no longer
used, you need to mark them deprecated.

In short, you need to decide what you're doing about logging, if anything (can't
we have something that improves the debug page and later decide which if any
fields we want to add to the logs), and about protocol buffers.  Once you decide
and resolve that, I'll take another look.

--mark


> 
> I am not satisfied with the table layout: I tried to restrict the width of the
> 'URL' column and failed. Perhaps you could suggest the proper fix. (I tried
> setting max-width, using a custom class, etc.) I think the 'URL' column being
to
> wide prevented the 'Logged Info' column from widening properly.

[mrossetti, 2012-07-03 21:19:00]

Updated by removing the dangerous third rail of logs.

[Mark P, 2012-07-03 21:26:42]

On 2012/07/03 21:19:00, mrossetti wrote:
> Updated by removing the dangerous third rail of logs.

Needs a bit of comment cleanup and the like but the structure looks good to me.
:)

I love your pithy summary of my logs comments.

--mark

[mrossetti, 2012-07-03 21:49:08]

On 2012/07/03 21:26:42, Mark P wrote:
> On 2012/07/03 21:19:00, mrossetti wrote:
> > Updated by removing the dangerous third rail of logs.
> 
> Needs a bit of comment cleanup and the like but the structure looks good to
me.
> :)
> 
> I love your pithy summary of my logs comments.
> 
> --mark

I hope you didn't think I was being snarky!

Any suggestions on how to get the URL column to be narrower so the 'Logged Info'
column can be wider?

Mike

[Mark P, 2012-07-09 20:16:02]

On 2012/07/03 21:49:08, mrossetti wrote:
> I hope you didn't think I was being snarky!

Only a little. ;)  But I'm not going to complain about snarkiness when it's
true.  I enjoyed your pithy summary of my message.

> Any suggestions on how to get the URL column to be narrower so the 'Logged
Info'
> column can be wider?

Perhaps simply elide some of the URL?  Have the URL only display the first
thirty characters then "..."   Because the URLs are clickable, people can hover
to see the whole thing if they wanted.

--mark

[mrossetti, 2012-07-09 23:18:24]

Ready for another look.

[Mark P, 2012-07-10 14:51:34]

https://chromiumcodereview.appspot.com/10692075/diff/21002/chrome/browser/aut...
File chrome/browser/autocomplete/autocomplete_match.h (right):

https://chromiumcodereview.appspot.com/10692075/diff/21002/chrome/browser/aut...
chrome/browser/autocomplete/autocomplete_match.h:198: // Adds logging
information to the logging dictionary |info_log|.
Please don't call this type of information (displayed on chrome://omnibox)
logged information.  It's not logged.

This comment requires changes in multiple places in multiple files.

https://chromiumcodereview.appspot.com/10692075/diff/21002/chrome/browser/aut...
chrome/browser/autocomplete/autocomplete_match.h:199: void LogInfo(const
std::string& title, const std::string& value);
Title feels to me a bit odd.  Consider something else like name or property or
feature or variable_name or ...

Once you rename this, please change the css/html/js appropriately.

https://chromiumcodereview.appspot.com/10692075/diff/21002/chrome/browser/res...
File chrome/browser/resources/omnibox/omnibox.js (right):

https://chromiumcodereview.appspot.com/10692075/diff/21002/chrome/browser/res...
chrome/browser/resources/omnibox/omnibox.js:67: *     presented as a tooltip.
Please mention hover here.

https://chromiumcodereview.appspot.com/10692075/diff/21002/chrome/browser/res...
chrome/browser/resources/omnibox/omnibox.js:91: 'The type of the results.'),
results -> result ?

[mrossetti, 2012-07-10 17:57:08]

Tres gut! Blotted out references to 'logging'.

http://codereview.chromium.org/10692075/diff/21002/chrome/browser/autocomplet...
File chrome/browser/autocomplete/autocomplete_match.h (right):

http://codereview.chromium.org/10692075/diff/21002/chrome/browser/autocomplet...
chrome/browser/autocomplete/autocomplete_match.h:198: // Adds logging
information to the logging dictionary |info_log|.
On 2012/07/10 14:51:34, Mark P wrote:
> Please don't call this type of information (displayed on chrome://omnibox)
> logged information.  It's not logged.
> 
> This comment requires changes in multiple places in multiple files.

Excellent suggestion! I want with forms of the word 'diagnostic'. What do you
think?

http://codereview.chromium.org/10692075/diff/21002/chrome/browser/autocomplet...
chrome/browser/autocomplete/autocomplete_match.h:199: void LogInfo(const
std::string& title, const std::string& value);
On 2012/07/10 14:51:34, Mark P wrote:
> Title feels to me a bit odd.  Consider something else like name or property or
> feature or variable_name or ...
> 
> Once you rename this, please change the css/html/js appropriately.

In this case, I went with 'property'.

http://codereview.chromium.org/10692075/diff/21002/chrome/browser/resources/o...
File chrome/browser/resources/omnibox/omnibox.js (right):

http://codereview.chromium.org/10692075/diff/21002/chrome/browser/resources/o...
chrome/browser/resources/omnibox/omnibox.js:67: *     presented as a tooltip.
On 2012/07/10 14:51:34, Mark P wrote:
> Please mention hover here.

Done.

http://codereview.chromium.org/10692075/diff/21002/chrome/browser/resources/o...
chrome/browser/resources/omnibox/omnibox.js:91: 'The type of the results.'),
On 2012/07/10 14:51:34, Mark P wrote:
> results -> result ?

Done.

[Mark P, 2012-07-10 19:41:12]

lgtm

You will need reviews from estade@ for omnibox_handler.cc and arv@ for the JS
and CSS and pkasting@ if you decide he's necessary for the autocomplete stuff.

FYI, arv@ tends to be miss or forget about code review e-mails.  I suggest that
if you add him to the review list and if you don't hear from him within a day or
two then you should explicitly contact him.

--mark

http://codereview.chromium.org/10692075/diff/10004/chrome/browser/autocomplet...
File chrome/browser/autocomplete/autocomplete_match.h (right):

http://codereview.chromium.org/10692075/diff/10004/chrome/browser/autocomplet...
chrome/browser/autocomplete/autocomplete_match.h:298: // record a property and
associated value.
Consider adding:
Used for chrome://omnibox

http://codereview.chromium.org/10692075/diff/10004/chrome/browser/resources/o...
File chrome/browser/resources/omnibox/omnibox.js (right):

http://codereview.chromium.org/10692075/diff/10004/chrome/browser/resources/o...
chrome/browser/resources/omnibox/omnibox.js:97: 'If checked the result has been
bookmarked.'),
Optional nit: sometimes below the tooltips are
If checked ...
and some are
Checked when ...
and one is
A green checkmark indicates ...

Consider making all these read similarly unless they read better in different
versions.

http://codereview.chromium.org/10692075/diff/10004/chrome/browser/resources/o...
chrome/browser/resources/omnibox/omnibox.js:102: 'The page title or suggestion
provider name.'),
This doesn't seem accurate.

http://codereview.chromium.org/10692075/diff/10004/chrome/browser/resources/o...
chrome/browser/resources/omnibox/omnibox.js:113: 'A green checkmkark indicates
that the result was'),
This doesn't actually say much of anything.

http://codereview.chromium.org/10692075/diff/10004/chrome/browser/resources/o...
chrome/browser/resources/omnibox/omnibox.js:127: 'Provider-specific information
about each suggestion.')
I'd prefer result instead of suggestion here.

Actually, in general we're sometimes saying suggestion in this tooltips and
sometimes saying result.  Consider making these consistent.

http://codereview.chromium.org/10692075/diff/10004/chrome/browser/resources/o...
chrome/browser/resources/omnibox/omnibox.js:173: // |diagnostics| presents a
two-column table of provider-specific data.
Perhaps:
presents -> embeds
and append:
within this cell.

http://codereview.chromium.org/10692075/diff/10004/chrome/browser/resources/o...
chrome/browser/resources/omnibox/omnibox.js:178: property_cell.textContent =
diagnostic_key + ':';
Does the ':' add to readability of the table?

[mrossetti, 2012-07-14 02:34:05]

arv: Would you please take a look at the chrome/browser/ui/webui and
chrome/browser/resources changes as one of the owners?

pkasting: I thought you might just want to take a quick look at these changes.

http://codereview.chromium.org/10692075/diff/10004/chrome/browser/autocomplet...
File chrome/browser/autocomplete/autocomplete_match.h (right):

http://codereview.chromium.org/10692075/diff/10004/chrome/browser/autocomplet...
chrome/browser/autocomplete/autocomplete_match.h:298: // record a property and
associated value.
On 2012/07/10 19:41:12, Mark P wrote:
> Consider adding:
> Used for chrome://omnibox

Done.

http://codereview.chromium.org/10692075/diff/10004/chrome/browser/resources/o...
File chrome/browser/resources/omnibox/omnibox.js (right):

http://codereview.chromium.org/10692075/diff/10004/chrome/browser/resources/o...
chrome/browser/resources/omnibox/omnibox.js:97: 'If checked the result has been
bookmarked.'),
On 2012/07/10 19:41:12, Mark P wrote:
> Optional nit: sometimes below the tooltips are
> If checked ...
> and some are
> Checked when ...
> and one is
> A green checkmark indicates ...
> 
> Consider making all these read similarly unless they read better in different
> versions.

Done.

http://codereview.chromium.org/10692075/diff/10004/chrome/browser/resources/o...
chrome/browser/resources/omnibox/omnibox.js:102: 'The page title or suggestion
provider name.'),
On 2012/07/10 19:41:12, Mark P wrote:
> This doesn't seem accurate.

Fixed.

http://codereview.chromium.org/10692075/diff/10004/chrome/browser/resources/o...
chrome/browser/resources/omnibox/omnibox.js:113: 'A green checkmkark indicates
that the result was'),
On 2012/07/10 19:41:12, Mark P wrote:
> This doesn't actually say much of anything.

I actually don't know what this column means so I cleared it for now.

http://codereview.chromium.org/10692075/diff/10004/chrome/browser/resources/o...
chrome/browser/resources/omnibox/omnibox.js:127: 'Provider-specific information
about each suggestion.')
On 2012/07/10 19:41:12, Mark P wrote:
> I'd prefer result instead of suggestion here.
> 
> Actually, in general we're sometimes saying suggestion in this tooltips and
> sometimes saying result.  Consider making these consistent.

Done.

http://codereview.chromium.org/10692075/diff/10004/chrome/browser/resources/o...
chrome/browser/resources/omnibox/omnibox.js:173: // |diagnostics| presents a
two-column table of provider-specific data.
On 2012/07/10 19:41:12, Mark P wrote:
> Perhaps:
> presents -> embeds
> and append:
> within this cell.

Done.

http://codereview.chromium.org/10692075/diff/10004/chrome/browser/resources/o...
chrome/browser/resources/omnibox/omnibox.js:178: property_cell.textContent =
diagnostic_key + ':';
On 2012/07/10 19:41:12, Mark P wrote:
> Does the ':' add to readability of the table?

Yes.

[mrossetti, 2012-07-24 17:11:45]

arv: Would you please take a quick look at the chrome/browser/ui/webui and
chrome/browser/resources changes as one of the owners?

pkasting: Are you interested in taking a quick look at these changes?

[mrossetti, 2012-07-24 18:45:32]

- arv
+ estade

estade: would you please give a quick owners once-over of the
chrome/browser/ui/webui and chrome/browser/resources changes?

pkasting: I need your LG for the autocomplete portions.

[Peter Kasting, 2012-07-26 02:31:37]

autocomplete LGTM

http://codereview.chromium.org/10692075/diff/36001/chrome/browser/autocomplet...
File chrome/browser/autocomplete/autocomplete_match.h (right):

http://codereview.chromium.org/10692075/diff/36001/chrome/browser/autocomplet...
chrome/browser/autocomplete/autocomplete_match.h:197: // Adds diagnostic
information to the |diagnostics| dictionary.
Nit: Up to you, but I think "optional info" might be a better name than
"diagnostics".  I don't feel strongly about this.

http://codereview.chromium.org/10692075/diff/36001/chrome/browser/autocomplet...
chrome/browser/autocomplete/autocomplete_match.h:299: std::map<std::string,
std::string> diagnostics;
Nit: Might want a "typedef std::map<std::string, std::string> OptionalInfo" (or
whatever).  This doesn't buy much because you only use the type in one other
place (omnibox_ui_handler.cc) so it's up to you.

[Evan Stade, 2012-07-26 03:56:21]

http://codereview.chromium.org/10692075/diff/36001/chrome/browser/resources/o...
File chrome/browser/resources/omnibox/omnibox.css (right):

http://codereview.chromium.org/10692075/diff/36001/chrome/browser/resources/o...
chrome/browser/resources/omnibox/omnibox.css:6: font-family: Arial, Helvetica,
sans-serif;
the font stuff should be set via SetFontAndTextDirection() in C++

http://codereview.chromium.org/10692075/diff/36001/chrome/browser/ui/webui/om...
File chrome/browser/ui/webui/omnibox/omnibox_ui_handler.cc (right):

http://codereview.chromium.org/10692075/diff/36001/chrome/browser/ui/webui/om...
chrome/browser/ui/webui/omnibox/omnibox_ui_handler.cc:129:
it->diagnostics.begin(); j != it->diagnostics.end(); ++j)
curlies

[Peter Kasting, 2012-07-26 05:25:42]

http://codereview.chromium.org/10692075/diff/36001/chrome/browser/ui/webui/om...
File chrome/browser/ui/webui/omnibox/omnibox_ui_handler.cc (right):

http://codereview.chromium.org/10692075/diff/36001/chrome/browser/ui/webui/om...
chrome/browser/ui/webui/omnibox/omnibox_ui_handler.cc:129:
it->diagnostics.begin(); j != it->diagnostics.end(); ++j)
On 2012/07/26 03:56:21, Evan Stade wrote:
> curlies

Not necessary (and I discourage them), since the body is one line.

[mrossetti, 2012-07-26 16:33:59]

Evan,

Please clarify this comment:

On 2012/07/26 03:56:21, Evan Stade wrote:
>
http://codereview.chromium.org/10692075/diff/36001/chrome/browser/resources/o...
> chrome/browser/resources/omnibox/omnibox.css:6: font-family: Arial, Helvetica,
> sans-serif;
> the font stuff should be set via SetFontAndTextDirection() in C++

I don't understand: are you saying that font-family and font-size should be set
with SetFontAndTextDirection()? I see plenty of examples of setting family and
size in many CSS files in Resources.

[arv (Not doing code reviews), 2012-07-26 21:02:00]

some more nits

http://codereview.chromium.org/10692075/diff/36001/chrome/browser/resources/o...
File chrome/browser/resources/omnibox/omnibox.css (right):

http://codereview.chromium.org/10692075/diff/36001/chrome/browser/resources/o...
chrome/browser/resources/omnibox/omnibox.css:46: .diagnostic-property {
.diagnostic-property,
.diagnostic-value {
  white-space: nowrap;
}

http://codereview.chromium.org/10692075/diff/36001/chrome/browser/resources/o...
File chrome/browser/resources/omnibox/omnibox.js (right):

http://codereview.chromium.org/10692075/diff/36001/chrome/browser/resources/o...
chrome/browser/resources/omnibox/omnibox.js:179: var diagnostic_table =
document.createElement('table');
no underscores in js

[mrossetti, 2012-07-26 23:17:13]

Thanks everyone -- I've hopefully take care of things to your satisfaction.

Evan: I have an outstanding question on your comment about using
SetFontAndTextDirection() in omnibox.css. Could you give me some clarification
on that please?

http://codereview.chromium.org/10692075/diff/36001/chrome/browser/autocomplet...
File chrome/browser/autocomplete/autocomplete_match.h (right):

http://codereview.chromium.org/10692075/diff/36001/chrome/browser/autocomplet...
chrome/browser/autocomplete/autocomplete_match.h:197: // Adds diagnostic
information to the |diagnostics| dictionary.
On 2012/07/26 02:31:37, Peter Kasting wrote:
> Nit: Up to you, but I think "optional info" might be a better name than
> "diagnostics".  I don't feel strongly about this.

I like that idea but I used |additional_info| -- I hope that's okay by you.

http://codereview.chromium.org/10692075/diff/36001/chrome/browser/autocomplet...
chrome/browser/autocomplete/autocomplete_match.h:299: std::map<std::string,
std::string> diagnostics;
On 2012/07/26 02:31:37, Peter Kasting wrote:
> Nit: Might want a "typedef std::map<std::string, std::string> OptionalInfo"
(or
> whatever).  This doesn't buy much because you only use the type in one other
> place (omnibox_ui_handler.cc) so it's up to you.

Done.

http://codereview.chromium.org/10692075/diff/36001/chrome/browser/resources/o...
File chrome/browser/resources/omnibox/omnibox.css (right):

http://codereview.chromium.org/10692075/diff/36001/chrome/browser/resources/o...
chrome/browser/resources/omnibox/omnibox.css:6: font-family: Arial, Helvetica,
sans-serif;
On 2012/07/26 03:56:21, Evan Stade wrote:
> the font stuff should be set via SetFontAndTextDirection() in C++

I'm sorry but I don't understand: are you saying that font-family and font-size
should be set with SetFontAndTextDirection()? I see plenty of examples of
setting family and size in many CSS files in Resources.

http://codereview.chromium.org/10692075/diff/36001/chrome/browser/resources/o...
chrome/browser/resources/omnibox/omnibox.css:46: .diagnostic-property {
On 2012/07/26 21:02:00, arv wrote:
> .diagnostic-property,
> .diagnostic-value {
>   white-space: nowrap;
> }

Done.

http://codereview.chromium.org/10692075/diff/36001/chrome/browser/resources/o...
File chrome/browser/resources/omnibox/omnibox.js (right):

http://codereview.chromium.org/10692075/diff/36001/chrome/browser/resources/o...
chrome/browser/resources/omnibox/omnibox.js:179: var diagnostic_table =
document.createElement('table');
On 2012/07/26 21:02:00, arv wrote:
> no underscores in js

Done throughout.

[Evan Stade, 2012-07-27 00:04:41]

http://codereview.chromium.org/10692075/diff/36001/chrome/browser/resources/o...
File chrome/browser/resources/omnibox/omnibox.css (right):

http://codereview.chromium.org/10692075/diff/36001/chrome/browser/resources/o...
chrome/browser/resources/omnibox/omnibox.css:6: font-family: Arial, Helvetica,
sans-serif;
On 2012/07/26 23:17:13, mrossetti wrote:
> On 2012/07/26 03:56:21, Evan Stade wrote:
> > the font stuff should be set via SetFontAndTextDirection() in C++
> 
> I'm sorry but I don't understand: are you saying that font-family and
font-size
> should be set with SetFontAndTextDirection()? I see plenty of examples of
> setting family and size in many CSS files in Resources.

they are wrong to do so, but they are older than SetFontAndTextDirection()

http://codereview.chromium.org/10692075/diff/36001/chrome/browser/ui/webui/om...
File chrome/browser/ui/webui/omnibox/omnibox_ui_handler.cc (right):

http://codereview.chromium.org/10692075/diff/36001/chrome/browser/ui/webui/om...
chrome/browser/ui/webui/omnibox/omnibox_ui_handler.cc:129:
it->diagnostics.begin(); j != it->diagnostics.end(); ++j)
On 2012/07/26 05:25:43, Peter Kasting wrote:
> On 2012/07/26 03:56:21, Evan Stade wrote:
> > curlies
> 
> Not necessary (and I discourage them), since the body is one line.

I don't like that rule for loops, and the style guide seems to recommend curlies
here:

"curly braces are not required for single-line statements, but they are allowed
if you like them; conditional or loop statements with complex conditions or
statements may be more readable with curly braces"

[mrossetti, 2012-07-27 17:55:36]

Evan, I've addressed both of your remaining comments.

If you're happy with these changes, I'd like to get an LG today as I will be OOO
next week and would prefer not to let these changes age too much more.

Thanks to everyone for their careful reviews!

http://codereview.chromium.org/10692075/diff/36001/chrome/browser/resources/o...
File chrome/browser/resources/omnibox/omnibox.css (right):

http://codereview.chromium.org/10692075/diff/36001/chrome/browser/resources/o...
chrome/browser/resources/omnibox/omnibox.css:6: font-family: Arial, Helvetica,
sans-serif;
On 2012/07/27 00:04:42, Evan Stade wrote:
> On 2012/07/26 23:17:13, mrossetti wrote:
> > On 2012/07/26 03:56:21, Evan Stade wrote:
> > > the font stuff should be set via SetFontAndTextDirection() in C++
> > 
> > I'm sorry but I don't understand: are you saying that font-family and
> font-size
> > should be set with SetFontAndTextDirection()? I see plenty of examples of
> > setting family and size in many CSS files in Resources.
> 
> they are wrong to do so, but they are older than SetFontAndTextDirection()

No problem. I removed the font stuff for now as it isn't central to this change.

http://codereview.chromium.org/10692075/diff/36001/chrome/browser/ui/webui/om...
File chrome/browser/ui/webui/omnibox/omnibox_ui_handler.cc (right):

http://codereview.chromium.org/10692075/diff/36001/chrome/browser/ui/webui/om...
chrome/browser/ui/webui/omnibox/omnibox_ui_handler.cc:129:
it->diagnostics.begin(); j != it->diagnostics.end(); ++j)
On 2012/07/27 00:04:42, Evan Stade wrote:
> On 2012/07/26 05:25:43, Peter Kasting wrote:
> > On 2012/07/26 03:56:21, Evan Stade wrote:
> > > curlies
> > 
> > Not necessary (and I discourage them), since the body is one line.
> 
> I don't like that rule for loops, and the style guide seems to recommend
curlies
> here:
> 
> "curly braces are not required for single-line statements, but they are
allowed
> if you like them; conditional or loop statements with complex conditions or
> statements may be more readable with curly braces"

Done.

[Evan Stade, 2012-07-28 01:04:32]

lgtm

http://codereview.chromium.org/10692075/diff/53003/chrome/browser/resources/o...
File chrome/browser/resources/omnibox/omnibox.js (right):

http://codereview.chromium.org/10692075/diff/53003/chrome/browser/resources/o...
chrome/browser/resources/omnibox/omnibox.js:191:
additionalInfoTable.appendChild(additionalInfoRow);
vertical space somewhere in here please

[mrossetti, 2012-07-28 21:37:31]

Flushing comments.

Committed.

http://codereview.chromium.org/10692075/diff/53003/chrome/browser/resources/o...
File chrome/browser/resources/omnibox/omnibox.js (right):

http://codereview.chromium.org/10692075/diff/53003/chrome/browser/resources/o...
chrome/browser/resources/omnibox/omnibox.js:191:
additionalInfoTable.appendChild(additionalInfoRow);
On 2012/07/28 01:04:32, Evan Stade wrote:
> vertical space somewhere in here please

Done.