Add (uneditable) display of extension keywords to the search engine manager.

Add (uneditable) display of extension keywords to the search engine manager.

R=jhawkins
BUG=73666
TEST=


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

[Greg Billock, 2011-06-10 22:18:27]

On 2011/06/10 22:11:43, gbillock1 wrote:

Added a comment on the bug thread with a screen shot of what this ends up
looking like.

[Greg Billock, 2011-06-23 20:02:13]

On 2011/06/10 22:18:27, Greg Billock wrote:
> On 2011/06/10 22:11:43, gbillock1 wrote:
> 
> Added a comment on the bug thread with a screen shot of what this ends up
> looking like.

OK, this is ready for review.

[James Hawkins, 2011-07-11 03:59:45]

On 2011/06/23 20:02:13, Greg Billock wrote:
> On 2011/06/10 22:18:27, Greg Billock wrote:
> > On 2011/06/10 22:11:43, gbillock1 wrote:
> > 
> > Added a comment on the bug thread with a screen shot of what this ends up
> > looking like.
> 
> OK, this is ready for review.

Can you show a before and after screenshot?

[James Hawkins, 2011-07-11 04:04:33]

http://codereview.chromium.org/7104121/diff/14001/chrome/browser/resources/op...
File chrome/browser/resources/options/search_engine_manager.css (right):

http://codereview.chromium.org/7104121/diff/14001/chrome/browser/resources/op...
chrome/browser/resources/options/search_engine_manager.css:65: visibility:
hidden;
Use the hidden attribute in the HTML.

http://codereview.chromium.org/7104121/diff/14001/chrome/browser/resources/op...
File chrome/browser/resources/options/search_engine_manager.html (right):

http://codereview.chromium.org/7104121/diff/14001/chrome/browser/resources/op...
chrome/browser/resources/options/search_engine_manager.html:9: <h3
i18n-content="extensionKeywordsListTitle" id="extensionKeywordListTitle"></h3>
80 cols

http://codereview.chromium.org/7104121/diff/14001/chrome/browser/resources/op...
chrome/browser/resources/options/search_engine_manager.html:9: <h3
i18n-content="extensionKeywordsListTitle" id="extensionKeywordListTitle"></h3>
IDs are dash-form, though I see now that this file hasn't been cleaned up yet.

http://codereview.chromium.org/7104121/diff/14001/chrome/browser/resources/op...
File chrome/browser/resources/options/search_engine_manager.js (left):

http://codereview.chromium.org/7104121/diff/14001/chrome/browser/resources/op...
chrome/browser/resources/options/search_engine_manager.js:27: * @type {boolean}
Why did you remove the type?

http://codereview.chromium.org/7104121/diff/14001/chrome/browser/resources/op...
File chrome/browser/resources/options/search_engine_manager.js (right):

http://codereview.chromium.org/7104121/diff/14001/chrome/browser/resources/op...
chrome/browser/resources/options/search_engine_manager.js:38: * List for
extension keywords
Missing period at end of sentence.

http://codereview.chromium.org/7104121/diff/14001/chrome/browser/resources/op...
chrome/browser/resources/options/search_engine_manager.js:92:
$('extensionKeywordListTitle').style.visibility = 'visible';
Use hidden attribute.

http://codereview.chromium.org/7104121/diff/14001/chrome/browser/ui/webui/opt...
File chrome/browser/ui/webui/options/search_engine_manager_handler.cc (right):

http://codereview.chromium.org/7104121/diff/14001/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options/search_engine_manager_handler.cc:144:
keyword_list.Append(CreateDictionaryForExtension(*(*it)));
Do you need the extra parens around *it?

http://codereview.chromium.org/7104121/diff/14001/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options/search_engine_manager_handler.cc:172:
dict->SetString("url", "");
s/""/string16()/

[Greg Billock, 2011-07-11 16:04:41]

Yes, it's in the bug thread (
http://code.google.com/p/chromium/issues/detail?id=73666)

On Sun, Jul 10, 2011 at 8:59 PM, <jhawkins@chromium.org> wrote:

> On 2011/06/23 20:02:13, Greg Billock wrote:
>
>> On 2011/06/10 22:18:27, Greg Billock wrote:
>> > On 2011/06/10 22:11:43, gbillock1 wrote:
>> >
>> > Added a comment on the bug thread with a screen shot of what this ends
>> up
>> > looking like.
>>
>
>  OK, this is ready for review.
>>
>
> Can you show a before and after screenshot?
>
>
http://codereview.chromium.**org/7104121/<http://codereview.chromium.org/7104...
>

[Evan Stade, 2011-07-11 20:12:01]

nits only, otherwise LGTM

http://codereview.chromium.org/7104121/diff/14001/chrome/browser/resources/op...
File chrome/browser/resources/options/search_engine_manager.js (right):

http://codereview.chromium.org/7104121/diff/14001/chrome/browser/resources/op...
chrome/browser/resources/options/search_engine_manager.js:94:
$('manageExtensionLink').style.visibility = 'visible';
I would wrap the whole extension section in a div to make hiding it (or
performing other similar actions) simpler

http://codereview.chromium.org/7104121/diff/14001/chrome/browser/ui/webui/opt...
File chrome/browser/ui/webui/options/search_engine_manager_handler.cc (right):

http://codereview.chromium.org/7104121/diff/14001/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options/search_engine_manager_handler.cc:139: if
(extension_service != NULL) {
just if (extension_service)

[Greg Billock, 2011-07-11 21:24:25]

http://codereview.chromium.org/7104121/diff/14001/chrome/browser/resources/op...
File chrome/browser/resources/options/search_engine_manager.css (right):

http://codereview.chromium.org/7104121/diff/14001/chrome/browser/resources/op...
chrome/browser/resources/options/search_engine_manager.css:65: visibility:
hidden;
On 2011/07/11 04:04:33, James Hawkins wrote:
> Use the hidden attribute in the HTML.

Done.

http://codereview.chromium.org/7104121/diff/14001/chrome/browser/resources/op...
File chrome/browser/resources/options/search_engine_manager.html (right):

http://codereview.chromium.org/7104121/diff/14001/chrome/browser/resources/op...
chrome/browser/resources/options/search_engine_manager.html:9: <h3
i18n-content="extensionKeywordsListTitle" id="extensionKeywordListTitle"></h3>
On 2011/07/11 04:04:33, James Hawkins wrote:
> 80 cols

Done.

http://codereview.chromium.org/7104121/diff/14001/chrome/browser/resources/op...
chrome/browser/resources/options/search_engine_manager.html:9: <h3
i18n-content="extensionKeywordsListTitle" id="extensionKeywordListTitle"></h3>
On 2011/07/11 04:04:33, James Hawkins wrote:
> IDs are dash-form, though I see now that this file hasn't been cleaned up yet.

I can do that, but perhaps in another cl.

http://codereview.chromium.org/7104121/diff/14001/chrome/browser/resources/op...
File chrome/browser/resources/options/search_engine_manager.js (left):

http://codereview.chromium.org/7104121/diff/14001/chrome/browser/resources/op...
chrome/browser/resources/options/search_engine_manager.js:27: * @type {boolean}
On 2011/07/11 04:04:33, James Hawkins wrote:
> Why did you remove the type?

It's not accurate. I'd guess it got copy-pasted from somewhere.

http://codereview.chromium.org/7104121/diff/14001/chrome/browser/resources/op...
File chrome/browser/resources/options/search_engine_manager.js (right):

http://codereview.chromium.org/7104121/diff/14001/chrome/browser/resources/op...
chrome/browser/resources/options/search_engine_manager.js:38: * List for
extension keywords
On 2011/07/11 04:04:33, James Hawkins wrote:
> Missing period at end of sentence.

Done. (and above)

http://codereview.chromium.org/7104121/diff/14001/chrome/browser/resources/op...
chrome/browser/resources/options/search_engine_manager.js:92:
$('extensionKeywordListTitle').style.visibility = 'visible';
On 2011/07/11 04:04:33, James Hawkins wrote:
> Use hidden attribute.

Done.

http://codereview.chromium.org/7104121/diff/14001/chrome/browser/resources/op...
chrome/browser/resources/options/search_engine_manager.js:94:
$('manageExtensionLink').style.visibility = 'visible';
On 2011/07/11 20:12:01, Evan Stade wrote:
> I would wrap the whole extension section in a div to make hiding it (or
> performing other similar actions) simpler

That makes sense. The reason I didn't do it is basically selectors in the
context CSS -- it doesn't then apply to the h3 and lists correctly. I fussed
with changing the scoping rules, but some are shared and I got squeamish and
quit fiddling around. There's an improvement to be made there, but it has fairly
large scope.

http://codereview.chromium.org/7104121/diff/14001/chrome/browser/ui/webui/opt...
File chrome/browser/ui/webui/options/search_engine_manager_handler.cc (right):

http://codereview.chromium.org/7104121/diff/14001/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options/search_engine_manager_handler.cc:139: if
(extension_service != NULL) {
On 2011/07/11 20:12:01, Evan Stade wrote:
> just if (extension_service)

Done.

http://codereview.chromium.org/7104121/diff/14001/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options/search_engine_manager_handler.cc:144:
keyword_list.Append(CreateDictionaryForExtension(*(*it)));
On 2011/07/11 04:04:33, James Hawkins wrote:
> Do you need the extra parens around *it?

I could try without, but it's already pretty hard to follow as is without mixing
precedence.

http://codereview.chromium.org/7104121/diff/14001/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options/search_engine_manager_handler.cc:172:
dict->SetString("url", "");
On 2011/07/11 04:04:33, James Hawkins wrote:
> s/""/string16()/

Done.

[Evan Stade, 2011-07-12 16:12:01]

ah yes, I see what you mean about the css. It appears there's room for cleanup
here:

currently:

div.page > h3 {
  font-size: 105%;
  font-weight: bold;
  margin: 20px 0 10px 0;
}

section > h3 {
  font-size: 105%;
  font-weight: bold;
  margin: 0;
  vertical-align: middle;
  width: 130px;
  -webkit-padding-end: 10px;
}

should be

h3 {
  font-size: 105%;
  font-weight: bold;
  margin: 20px 0 10px 0;
}

section > h3 {
  margin: 0;
  vertical-align: middle;
  width: 130px;
  -webkit-padding-end: 10px;
}

http://codereview.chromium.org/7104121/diff/19001/chrome/browser/resources/op...
File chrome/browser/resources/options/search_engine_manager.html (right):

http://codereview.chromium.org/7104121/diff/19001/chrome/browser/resources/op...
chrome/browser/resources/options/search_engine_manager.html:9: <h3
i18n-content="extensionKeywordsListTitle" id="extensionKeywordListTitle"
id first

[Greg Billock, 2011-07-12 18:46:35]

Evan, I think you're right about the CSS, but let's change it in another cl
focused on that, since it cuts across a bunch of pages. (And perhaps there's
more selector specificity changes as well once we look.)

http://codereview.chromium.org/7104121/diff/19001/chrome/browser/resources/op...
File chrome/browser/resources/options/search_engine_manager.html (right):

http://codereview.chromium.org/7104121/diff/19001/chrome/browser/resources/op...
chrome/browser/resources/options/search_engine_manager.html:9: <h3
i18n-content="extensionKeywordsListTitle" id="extensionKeywordListTitle"
On 2011/07/12 16:12:01, Evan Stade wrote:
> id first

Done.

[commit-bot: I haz the power, 2011-07-12 20:58:56]

Try job failure for 7104121-19001 (retry) on linux for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux&numb...

[Evan Stade, 2011-07-12 21:52:39]

ok. Another CL to follow up on the cleanups suggested in this review
sounds fine.

-- Evan Stade



On Tue, Jul 12, 2011 at 11:46 AM,  <gbillock@chromium.org> wrote:
> Evan, I think you're right about the CSS, but let's change it in another cl
> focused on that, since it cuts across a bunch of pages. (And perhaps there's
> more selector specificity changes as well once we look.)
>
>
>
http://codereview.chromium.org/7104121/diff/19001/chrome/browser/resources/op...
> File chrome/browser/resources/options/search_engine_manager.html
> (right):
>
>
http://codereview.chromium.org/7104121/diff/19001/chrome/browser/resources/op...
> chrome/browser/resources/options/search_engine_manager.html:9: <h3
> i18n-content="extensionKeywordsListTitle" id="extensionKeywordListTitle"
> On 2011/07/12 16:12:01, Evan Stade wrote:
>>
>> id first
>
> Done.
>
> http://codereview.chromium.org/7104121/
>

[James Hawkins, 2011-07-12 21:54:36]

LGTM with nit.

http://codereview.chromium.org/7104121/diff/24002/chrome/browser/resources/op...
File chrome/browser/resources/options/search_engine_manager.html (right):

http://codereview.chromium.org/7104121/diff/24002/chrome/browser/resources/op...
chrome/browser/resources/options/search_engine_manager.html:10:
hidden="true"></h3>
s/="true"// here and elsewhere.

[Greg Billock, 2011-07-12 22:08:43]

http://codereview.chromium.org/7104121/diff/24002/chrome/browser/resources/op...
File chrome/browser/resources/options/search_engine_manager.html (right):

http://codereview.chromium.org/7104121/diff/24002/chrome/browser/resources/op...
chrome/browser/resources/options/search_engine_manager.html:10:
hidden="true"></h3>
On 2011/07/12 21:54:37, James Hawkins wrote:
> s/="true"// here and elsewhere.

Done.

[commit-bot: I haz the power, 2011-07-12 23:41:36]

Try job failure for 7104121-29002 (retry) on linux for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux&numb...

[commit-bot: I haz the power, 2011-07-13 22:08:22]

Change committed as 92431