InstantExtended: local omnibox popup fixes.

InstantExtended: local omnibox popup fixes.

Fix logic used to highlight suggestion and properly serve icons used in the
dropdown.  Also fix the blue highlight so it only extends to the end of the
omnibox.

BUG=196240, 196242
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=188947

[samarth, 2013-03-15 00:33:34]

Please take a look
estade -> js changes
sreeram -> instant
jered -> double check new logic

Thanks!
Samarth

[Jered, 2013-03-15 17:05:06]

A few quick questions

https://codereview.chromium.org/12720004/diff/1/chrome/browser/instant/local_...
File chrome/browser/instant/local_omnibox_popup_source.cc (right):

https://codereview.chromium.org/12720004/diff/1/chrome/browser/instant/local_...
chrome/browser/instant/local_omnibox_popup_source.cc:22: const char
kIcon2xFilename[] = "images/page_icon.png";
Should these file names be the same?

https://codereview.chromium.org/12720004/diff/1/chrome/browser/resources/loca...
File chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js (left):

https://codereview.chromium.org/12720004/diff/1/chrome/browser/resources/loca...
chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js:186: // Used
to workaround repeated undesired asynchronous onnativesuggestions
Is it ok to remove this now?

https://codereview.chromium.org/12720004/diff/1/chrome/browser/resources/loca...
File chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js
(right):

https://codereview.chromium.org/12720004/diff/1/chrome/browser/resources/loca...
chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js:222: if
(shouldSelectSuggestion(nativeSuggestions[0], apiHandle.verbatim)) {
Reset the selectedIndex to -1 otherwise.

https://codereview.chromium.org/12720004/diff/1/chrome/browser/resources/loca...
chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js:303:
apiHandle.onchange = updateSuggestions;
Why does this also need to happen onchange?

[samarth, 2013-03-15 21:58:27]

https://codereview.chromium.org/12720004/diff/1/chrome/browser/instant/local_...
File chrome/browser/instant/local_omnibox_popup_source.cc (right):

https://codereview.chromium.org/12720004/diff/1/chrome/browser/instant/local_...
chrome/browser/instant/local_omnibox_popup_source.cc:22: const char
kIcon2xFilename[] = "images/page_icon.png";
On 2013/03/15 17:05:06, Jered wrote:
> Should these file names be the same?

Oops, yeah fixed.

https://codereview.chromium.org/12720004/diff/1/chrome/browser/resources/loca...
File chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js (left):

https://codereview.chromium.org/12720004/diff/1/chrome/browser/resources/loca...
chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js:186: // Used
to workaround repeated undesired asynchronous onnativesuggestions
On 2013/03/15 17:05:06, Jered wrote:
> Is it ok to remove this now?

Talked to Jeremy who wrote this and this was just a workaround for the clicking
on suggestions bug which you just fixed.

https://codereview.chromium.org/12720004/diff/1/chrome/browser/resources/loca...
File chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js
(right):

https://codereview.chromium.org/12720004/diff/1/chrome/browser/resources/loca...
chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js:222: if
(shouldSelectSuggestion(nativeSuggestions[0], apiHandle.verbatim)) {
On 2013/03/15 17:05:06, Jered wrote:
> Reset the selectedIndex to -1 otherwise.

Already being done in clearSuggestions above. Would you prefer to do it
explicitly here?

https://codereview.chromium.org/12720004/diff/1/chrome/browser/resources/loca...
chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js:303:
apiHandle.onchange = updateSuggestions;
On 2013/03/15 17:05:06, Jered wrote:
> Why does this also need to happen onchange?

AFAICT, hitting backspace does not trigger onnativesuggestions, only an
onchange.

[Jered, 2013-03-15 22:04:09]

lgtm

https://codereview.chromium.org/12720004/diff/1/chrome/browser/resources/loca...
File chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js
(right):

https://codereview.chromium.org/12720004/diff/1/chrome/browser/resources/loca...
chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js:222: if
(shouldSelectSuggestion(nativeSuggestions[0], apiHandle.verbatim)) {
On 2013/03/15 21:58:27, samarth wrote:
> On 2013/03/15 17:05:06, Jered wrote:
> > Reset the selectedIndex to -1 otherwise.
> 
> Already being done in clearSuggestions above. Would you prefer to do it
> explicitly here?

Oh, I missed that. This way is fine.

[Shishir, 2013-03-15 22:50:48]

On 2013/03/15 22:04:09, Jered wrote:
> lgtm
> 
>
https://codereview.chromium.org/12720004/diff/1/chrome/browser/resources/loca...
> File chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js
> (right):
> 
>
https://codereview.chromium.org/12720004/diff/1/chrome/browser/resources/loca...
> chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js:222: if
> (shouldSelectSuggestion(nativeSuggestions[0], apiHandle.verbatim)) {
> On 2013/03/15 21:58:27, samarth wrote:
> > On 2013/03/15 17:05:06, Jered wrote:
> > > Reset the selectedIndex to -1 otherwise.
> > 
> > Already being done in clearSuggestions above. Would you prefer to do it
> > explicitly here?
> 
> Oh, I missed that. This way is fine.

Oops. Sorry I missed these in my CL. And thanks for the fix.

lgtm

[samarth, 2013-03-18 22:20:11]

sreeram -> dhollowa to load balance

[dhollowa, 2013-03-18 22:23:07]

lgtm

[samarth, 2013-03-18 23:24:10]

estade: friendly ping.

Thanks!
Samarth

[Evan Stade, 2013-03-18 23:53:05]

lgtm

[commit-bot: I haz the power, 2013-03-19 00:11:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/samarth@chromium.org/12720004/22001

[commit-bot: I haz the power, 2013-03-19 00:11:52]

Presubmit check for 12720004-22001 failed and returned exit status 1.

INFO:root:Found 3 file(s).

Running presubmit commit checks ...
Running /b/commit-queue/workdir/chromium/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/chrome/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/chrome/browser/resources/PRESUBMIT.py

** Presubmit Messages **
Was the CSS checker useful? Send feedback or hate mail to dbeam@chromium.org.

** Presubmit Warnings **
chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.css:
- Classes use .dash-form.
    .chrome_url {
    .chrome_title {

Presubmit checks took 1.6s to calculate.

[commit-bot: I haz the power, 2013-03-19 01:26:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/samarth@chromium.org/12720004/26001

[commit-bot: I haz the power, 2013-03-19 03:52:09]

Change committed as 188947