WebUI Edit Search Engine Dialog

WebUI Edit Search Engine Dialog

This version of the WebUI Edit Search Engine Dialog has everything except unit tests.

BUG=97846
TEST=Add Search Engine Manually

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

[wyck, 2011-10-04 19:39:07]

Hello!  Please review my conversion of the Edit Search Engine Dialog to a WebUI
Dialog.

[arv (Not doing code reviews), 2011-10-04 20:55:01]

A bunch of style issues.

http://codereview.chromium.org/8118012/diff/7016/chrome/browser/resources/edi...
File chrome/browser/resources/edit_search_engine_dialog.css (right):

http://codereview.chromium.org/8118012/diff/7016/chrome/browser/resources/edi...
chrome/browser/resources/edit_search_engine_dialog.css:42: -webkit-box-sizing:
border-box;
skip -webkit-box-sizing

http://codereview.chromium.org/8118012/diff/7016/chrome/browser/resources/edi...
chrome/browser/resources/edit_search_engine_dialog.css:51: float: right;
don't use floats. What is your intention here?

http://codereview.chromium.org/8118012/diff/7016/chrome/browser/resources/edi...
chrome/browser/resources/edit_search_engine_dialog.css:53: margin-right: 8px;
RTL

http://codereview.chromium.org/8118012/diff/7016/chrome/browser/resources/edi...
File chrome/browser/resources/edit_search_engine_dialog.html (right):

http://codereview.chromium.org/8118012/diff/7016/chrome/browser/resources/edi...
chrome/browser/resources/edit_search_engine_dialog.html:5: <title
id="title"></title>
Useless id. You can use document.title from script.

http://codereview.chromium.org/8118012/diff/7016/chrome/browser/resources/edi...
File chrome/browser/resources/edit_search_engine_dialog.js (right):

http://codereview.chromium.org/8118012/diff/7016/chrome/browser/resources/edi...
chrome/browser/resources/edit_search_engine_dialog.js:19: *        value to pass
to the dialog close handler.
indent 4 spaces

http://codereview.chromium.org/8118012/diff/7016/chrome/browser/resources/edi...
chrome/browser/resources/edit_search_engine_dialog.js:21: function
closeWithResult(result) {
opt_result

http://codereview.chromium.org/8118012/diff/7016/chrome/browser/resources/edi...
chrome/browser/resources/edit_search_engine_dialog.js:23: var json =
JSON.stringify(result?[result]:[]);
ws

http://codereview.chromium.org/8118012/diff/7016/chrome/browser/resources/edi...
chrome/browser/resources/edit_search_engine_dialog.js:28: * Sets the text of the
dialog's editable text boxes.
fix identation

http://codereview.chromium.org/8118012/diff/7016/chrome/browser/resources/edi...
chrome/browser/resources/edit_search_engine_dialog.js:44: $('title').textContent
= (chrome.dialogArguments == 'add') ?
useless parens

http://codereview.chromium.org/8118012/diff/7016/chrome/browser/resources/edi...
chrome/browser/resources/edit_search_engine_dialog.js:44: $('title').textContent
= (chrome.dialogArguments == 'add') ?
document.title = ...

http://codereview.chromium.org/8118012/diff/7016/chrome/browser/resources/edi...
chrome/browser/resources/edit_search_engine_dialog.js:52:
closeWithResult({description:$('description-text').value,
ws

http://codereview.chromium.org/8118012/diff/7016/chrome/browser/ui/browser.cc
File chrome/browser/ui/browser.cc (right):

http://codereview.chromium.org/8118012/diff/7016/chrome/browser/ui/browser.cc...
chrome/browser/ui/browser.cc:3911: }
remove return and use else

http://codereview.chromium.org/8118012/diff/7016/chrome/browser/ui/webui/edit...
File chrome/browser/ui/webui/edit_search_engine_dialog.h (right):

http://codereview.chromium.org/8118012/diff/7016/chrome/browser/ui/webui/edit...
chrome/browser/ui/webui/edit_search_engine_dialog.h:54: Profile* profile);
indentation

http://codereview.chromium.org/8118012/diff/7016/chrome/browser/ui/webui/edit...
File chrome/browser/ui/webui/edit_search_engine_dialog_ui.cc (right):

http://codereview.chromium.org/8118012/diff/7016/chrome/browser/ui/webui/edit...
chrome/browser/ui/webui/edit_search_engine_dialog_ui.cc:23:
source->AddLocalizedString("description-label",
descriptionLabel

http://dev.chromium.org/developers/web-development-style-guide

[wyck, 2011-10-04 21:57:57]

PTAL.

http://codereview.chromium.org/8118012/diff/7016/chrome/browser/resources/edi...
File chrome/browser/resources/edit_search_engine_dialog.css (right):

http://codereview.chromium.org/8118012/diff/7016/chrome/browser/resources/edi...
chrome/browser/resources/edit_search_engine_dialog.css:42: -webkit-box-sizing:
border-box;
On 2011/10/04 20:55:01, arv wrote:
> skip -webkit-box-sizing

Done.

http://codereview.chromium.org/8118012/diff/7016/chrome/browser/resources/edi...
chrome/browser/resources/edit_search_engine_dialog.css:51: float: right;
On 2011/10/04 20:55:01, arv wrote:
> don't use floats. What is your intention here?

My intention is that the buttons are at the right.  And the two buttons (cancel
and save) should be the same width regardless of language.

http://codereview.chromium.org/8118012/diff/7016/chrome/browser/resources/edi...
chrome/browser/resources/edit_search_engine_dialog.css:53: margin-right: 8px;
On 2011/10/04 20:55:01, arv wrote:
> RTL

Done.

http://codereview.chromium.org/8118012/diff/7016/chrome/browser/resources/edi...
File chrome/browser/resources/edit_search_engine_dialog.html (right):

http://codereview.chromium.org/8118012/diff/7016/chrome/browser/resources/edi...
chrome/browser/resources/edit_search_engine_dialog.html:5: <title
id="title"></title>
On 2011/10/04 20:55:01, arv wrote:
> Useless id. You can use document.title from script.

Done.

http://codereview.chromium.org/8118012/diff/7016/chrome/browser/resources/edi...
File chrome/browser/resources/edit_search_engine_dialog.js (right):

http://codereview.chromium.org/8118012/diff/7016/chrome/browser/resources/edi...
chrome/browser/resources/edit_search_engine_dialog.js:19: *        value to pass
to the dialog close handler.
On 2011/10/04 20:55:01, arv wrote:
> indent 4 spaces

Done.

http://codereview.chromium.org/8118012/diff/7016/chrome/browser/resources/edi...
chrome/browser/resources/edit_search_engine_dialog.js:21: function
closeWithResult(result) {
On 2011/10/04 20:55:01, arv wrote:
> opt_result

http://codereview.chromium.org/8118012/diff/7016/chrome/browser/resources/edi...
chrome/browser/resources/edit_search_engine_dialog.js:23: var json =
JSON.stringify(result?[result]:[]);
On 2011/10/04 20:55:01, arv wrote:
> ws

Done.

http://codereview.chromium.org/8118012/diff/7016/chrome/browser/resources/edi...
chrome/browser/resources/edit_search_engine_dialog.js:28: * Sets the text of the
dialog's editable text boxes.
On 2011/10/04 20:55:01, arv wrote:
> fix identation

Done.

http://codereview.chromium.org/8118012/diff/7016/chrome/browser/resources/edi...
chrome/browser/resources/edit_search_engine_dialog.js:44: $('title').textContent
= (chrome.dialogArguments == 'add') ?
On 2011/10/04 20:55:01, arv wrote:
> useless parens

Done.

http://codereview.chromium.org/8118012/diff/7016/chrome/browser/resources/edi...
chrome/browser/resources/edit_search_engine_dialog.js:44: $('title').textContent
= (chrome.dialogArguments == 'add') ?
On 2011/10/04 20:55:01, arv wrote:
> document.title = ...

Done.

http://codereview.chromium.org/8118012/diff/7016/chrome/browser/resources/edi...
chrome/browser/resources/edit_search_engine_dialog.js:52:
closeWithResult({description:$('description-text').value,
On 2011/10/04 20:55:01, arv wrote:
> ws
Not sure what it should be.

http://codereview.chromium.org/8118012/diff/7016/chrome/browser/ui/browser.cc
File chrome/browser/ui/browser.cc (right):

http://codereview.chromium.org/8118012/diff/7016/chrome/browser/ui/browser.cc...
chrome/browser/ui/browser.cc:3911: }
On 2011/10/04 20:55:01, arv wrote:
> remove return and use else

Done.

http://codereview.chromium.org/8118012/diff/7016/chrome/browser/ui/webui/edit...
File chrome/browser/ui/webui/edit_search_engine_dialog.h (right):

http://codereview.chromium.org/8118012/diff/7016/chrome/browser/ui/webui/edit...
chrome/browser/ui/webui/edit_search_engine_dialog.h:54: Profile* profile);
On 2011/10/04 20:55:01, arv wrote:
> indentation

Done.

http://codereview.chromium.org/8118012/diff/7016/chrome/browser/ui/webui/edit...
File chrome/browser/ui/webui/edit_search_engine_dialog_ui.cc (right):

http://codereview.chromium.org/8118012/diff/7016/chrome/browser/ui/webui/edit...
chrome/browser/ui/webui/edit_search_engine_dialog_ui.cc:23:
source->AddLocalizedString("description-label",
On 2011/10/04 20:55:01, arv wrote:
> descriptionLabel
> 
> http://dev.chromium.org/developers/web-development-style-guide

Done.

[flackr, 2011-10-05 02:41:11]

http://codereview.chromium.org/8118012/diff/7016/chrome/browser/resources/edi...
File chrome/browser/resources/edit_search_engine_dialog.css (right):

http://codereview.chromium.org/8118012/diff/7016/chrome/browser/resources/edi...
chrome/browser/resources/edit_search_engine_dialog.css:6: html, body {
Seems like this shouldn't be necessary on both html and body.

http://codereview.chromium.org/8118012/diff/7016/chrome/browser/resources/edi...
chrome/browser/resources/edit_search_engine_dialog.css:7: background-color:
#EEE;
s/EEE/eee

http://codereview.chromium.org/8118012/diff/9002/chrome/browser/resources/edi...
File chrome/browser/resources/edit_search_engine_dialog.css (right):

http://codereview.chromium.org/8118012/diff/9002/chrome/browser/resources/edi...
chrome/browser/resources/edit_search_engine_dialog.css:12: 
It's not clear to me that you've used .vbox, or .fill. If not remove them
please.

http://codereview.chromium.org/8118012/diff/9002/chrome/browser/ui/webui/edit...
File chrome/browser/ui/webui/edit_search_engine_dialog.h (right):

http://codereview.chromium.org/8118012/diff/9002/chrome/browser/ui/webui/edit...
chrome/browser/ui/webui/edit_search_engine_dialog.h:19: 
Class comments.

[wyck, 2011-10-05 14:32:32]

Addressed flackr's comments.

http://codereview.chromium.org/8118012/diff/7016/chrome/browser/resources/edi...
File chrome/browser/resources/edit_search_engine_dialog.css (right):

http://codereview.chromium.org/8118012/diff/7016/chrome/browser/resources/edi...
chrome/browser/resources/edit_search_engine_dialog.css:6: html, body {
On 2011/10/05 02:41:12, flackr wrote:
> Seems like this shouldn't be necessary on both html and body.

This is the way it was suggested to me originally.  Which one should it be then?
 html or body?

http://codereview.chromium.org/8118012/diff/7016/chrome/browser/resources/edi...
chrome/browser/resources/edit_search_engine_dialog.css:7: background-color:
#EEE;
On 2011/10/05 02:41:12, flackr wrote:
> s/EEE/eee

Done.

http://codereview.chromium.org/8118012/diff/9002/chrome/browser/resources/edi...
File chrome/browser/resources/edit_search_engine_dialog.css (right):

http://codereview.chromium.org/8118012/diff/9002/chrome/browser/resources/edi...
chrome/browser/resources/edit_search_engine_dialog.css:12: 
On 2011/10/05 02:41:12, flackr wrote:
> It's not clear to me that you've used .vbox, or .fill. If not remove them
> please.

Done.

http://codereview.chromium.org/8118012/diff/9002/chrome/browser/ui/webui/edit...
File chrome/browser/ui/webui/edit_search_engine_dialog.h (right):

http://codereview.chromium.org/8118012/diff/9002/chrome/browser/ui/webui/edit...
chrome/browser/ui/webui/edit_search_engine_dialog.h:19: 
On 2011/10/05 02:41:12, flackr wrote:
> Class comments.

Done.

[wyck, 2011-10-06 14:38:28]

ping?

[arv (Not doing code reviews), 2011-10-06 17:39:51]

http://codereview.chromium.org/8118012/diff/7016/chrome/browser/resources/edi...
File chrome/browser/resources/edit_search_engine_dialog.css (right):

http://codereview.chromium.org/8118012/diff/7016/chrome/browser/resources/edi...
chrome/browser/resources/edit_search_engine_dialog.css:51: float: right;
On 2011/10/04 21:57:57, wyck wrote:
> On 2011/10/04 20:55:01, arv wrote:
> > don't use floats. What is your intention here?
> 
> My intention is that the buttons are at the right.  And the two buttons
(cancel
> and save) should be the same width regardless of language.

Sorry for being slow here.

If I understand correctly the buttons are stacked vertically and they need to be
the same size.

The stacking and sizing is better done using flex box (direction vertical).

The right alignment is better done using text-align + inline-block or margin.
Floats should not be used for layout. Only for getting texts to wrap around.

You also need to use direction neutral properties (like -webkit-margin-start,
text-align: end etc).

Here is a quick prototype:

http://plexode.com/eval3/#ht=%3Cstyle%3E%0A%0A.a%20%7B%0A%20%20display%3A%20-...

[wyck, 2011-10-07 12:39:39]

On 2011/10/06 17:39:51, arv wrote:
>
http://codereview.chromium.org/8118012/diff/7016/chrome/browser/resources/edi...
> File chrome/browser/resources/edit_search_engine_dialog.css (right):
> 
>
http://codereview.chromium.org/8118012/diff/7016/chrome/browser/resources/edi...
> chrome/browser/resources/edit_search_engine_dialog.css:51: float: right;
> On 2011/10/04 21:57:57, wyck wrote:
> > On 2011/10/04 20:55:01, arv wrote:
> > > don't use floats. What is your intention here?
> > 
> > My intention is that the buttons are at the right.  And the two buttons
> (cancel
> > and save) should be the same width regardless of language.
> 
> Sorry for being slow here.
> 
> If I understand correctly the buttons are stacked vertically and they need to
be
> the same size.
> 
> The stacking and sizing is better done using flex box (direction vertical).
> 
> The right alignment is better done using text-align + inline-block or margin.
> Floats should not be used for layout. Only for getting texts to wrap around.
> 
> You also need to use direction neutral properties (like -webkit-margin-start,
> text-align: end etc).
> 
> Here is a quick prototype:
> 
>
http://plexode.com/eval3/#ht=%253Cstyle%253E%250A%250A.a%2520%257B%250A%2520%...

No, sorry for the misunderstanding, but the buttons are not supposed to be
stacked vertically.  They are side-by-side like a traditional OK|Cancel dialog
box.  But yes, they are supposed to be the same size.


This example you have provided looks like this:
      |A            |
      |Longer Button|

But what I need is this:
      |A            ||Longer Button|

And if I change your example to -webkit-orient-box: horizontal; then I get:
      |A||Longer Button|

Your suggestion doesn't work the way I need it to.  I had to use the table to
get the buttons the same size.  

If you know of a way to fix it, please let me know.

[arv (Not doing code reviews), 2011-10-10 17:37:33]

Please look at how this is done in other parts of WebUI.

We need to support flipping the order too and creating a new way to do this just
leads to harder to maintain code.

http://www.google.com/codesearch#OAMlx_jo-ck/src/chrome/browser/resources/opt...

http://www.google.com/codesearch#OAMlx_jo-ck/src/chrome/browser/resources/opt...

[wyck, 2011-10-11 15:32:46]

OK.  Thanks for the notes.  How's this?

[arv (Not doing code reviews), 2011-10-11 17:56:08]

You also need to flip the order of the buttons based on the os.

It would be nice if this code could be extracted into a shared "component".

http://codereview.chromium.org/8118012/diff/29001/chrome/browser/resources/ed...
File chrome/browser/resources/edit_search_engine_dialog.js (right):

http://codereview.chromium.org/8118012/diff/29001/chrome/browser/resources/ed...
chrome/browser/resources/edit_search_engine_dialog.js:107:
document.addEventListener('DOMContentLoaded',
I would keep this inside the closure above, then you do not need to export the
initialize funtion.

[flackr, 2011-10-11 18:05:35]

http://codereview.chromium.org/8118012/diff/29001/chrome/browser/resources/ed...
File chrome/browser/resources/edit_search_engine_dialog.html (right):

http://codereview.chromium.org/8118012/diff/29001/chrome/browser/resources/ed...
chrome/browser/resources/edit_search_engine_dialog.html:41: </div>
To support RTL and have minimal markup, would this work?
http://jsfiddle.net/u5JKF/

HTML:
<div id="action-area">
  <button>Connect</button>
  <button>Cancel</button>
</div>

CSS:
#action-area {
  text-align: end;   
}

[arv (Not doing code reviews), 2011-10-11 18:28:30]

http://codereview.chromium.org/8118012/diff/29001/chrome/browser/resources/ed...
File chrome/browser/resources/edit_search_engine_dialog.html (right):

http://codereview.chromium.org/8118012/diff/29001/chrome/browser/resources/ed...
chrome/browser/resources/edit_search_engine_dialog.html:41: </div>
On 2011/10/11 18:05:35, flackr wrote:
> To support RTL and have minimal markup, would this work?
> http://jsfiddle.net/u5JKF/
> 
> HTML:
> <div id="action-area">
>   <button>Connect</button>
>   <button>Cancel</button>
> </div>
> 
> CSS:
> #action-area {
>   text-align: end;   
> }

Yes.

But we also need to change the order based on the OS.

[wyck, 2011-10-11 18:39:51]

On 2011/10/11 17:56:08, arv wrote:
> You also need to flip the order of the buttons based on the os.
> 
> It would be nice if this code could be extracted into a shared "component".
> 
>
http://codereview.chromium.org/8118012/diff/29001/chrome/browser/resources/ed...
> File chrome/browser/resources/edit_search_engine_dialog.js (right):
> 
>
http://codereview.chromium.org/8118012/diff/29001/chrome/browser/resources/ed...
> chrome/browser/resources/edit_search_engine_dialog.js:107:
> document.addEventListener('DOMContentLoaded',
> I would keep this inside the closure above, then you do not need to export the
> initialize funtion.

You're going to have to be more specific about "flip the order of the buttons
based on the os".  I see the function OptionsPage.reverseButtonStrip_.  Is this
related?

[arv (Not doing code reviews), 2011-10-11 18:49:51]

On Tue, Oct 11, 2011 at 11:39,  <wyck@chromium.org> wrote:
> You're going to have to be more specific about "flip the order of the
> buttons
> based on the os".  I see the function OptionsPage.reverseButtonStrip_.  Is
> this
> related?

Sorry, yes, the OptionsPage.reverseButtonStrip_ function is what I was
referring to.

-- 
erik

[wyck, 2011-10-11 20:54:40]

PTAL.  And please double-check my jsdoc.  I have no experience with jsdoc.

[arv (Not doing code reviews), 2011-10-11 21:41:06]

http://codereview.chromium.org/8118012/diff/25010/chrome/browser/resources/ed...
File chrome/browser/resources/edit_search_engine_dialog.js (right):

http://codereview.chromium.org/8118012/diff/25010/chrome/browser/resources/ed...
chrome/browser/resources/edit_search_engine_dialog.js:90: function
filter(collection, fn) {
var filter = Array.prototype.filter.call.bind(Array.prototype.filter);

or use 

Array.prototype.filter.call(collection, func)

http://codereview.chromium.org/8118012/diff/25010/chrome/browser/resources/ed...
chrome/browser/resources/edit_search_engine_dialog.js:90: function
filter(collection, fn) {
Did you mean forEach here?

http://codereview.chromium.org/8118012/diff/25010/chrome/browser/resources/ed...
chrome/browser/resources/edit_search_engine_dialog.js:120:
filter(document.getElementsByClassName('button-strip'), reverseChildren);
document.qeurySelectorAll('.button-strip')

[wyck, 2011-10-12 13:52:59]

One more time, please.

http://codereview.chromium.org/8118012/diff/25010/chrome/browser/resources/ed...
File chrome/browser/resources/edit_search_engine_dialog.js (right):

http://codereview.chromium.org/8118012/diff/25010/chrome/browser/resources/ed...
chrome/browser/resources/edit_search_engine_dialog.js:80:
node.appendChild(childNodes[i]);
oops.  node instead of parent.  I could use some tests.

http://codereview.chromium.org/8118012/diff/25010/chrome/browser/resources/ed...
chrome/browser/resources/edit_search_engine_dialog.js:90: function
filter(collection, fn) {
On 2011/10/11 21:41:06, arv wrote:
> Did you mean forEach here?

Yes, thanks.

[arv (Not doing code reviews), 2011-10-12 17:44:20]

lgtm

[flackr, 2011-10-12 17:45:57]

lgtm

[commit-bot: I haz the power, 2011-10-12 17:47:47]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wyck@chromium.org/8118012/33017

[commit-bot: I haz the power, 2011-10-12 19:01:42]

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

[commit-bot: I haz the power, 2011-10-12 19:25:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wyck@chromium.org/8118012/38002

[commit-bot: I haz the power, 2011-10-12 20:31:32]

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

[wyck, 2011-10-13 21:36:40]

HELP!  I don't understand how I caused the link error on win_rel.  If you see
what's wrong, can you please let me know?

[flackr, 2011-10-18 21:26:49]

I'm really glad you figured out the Windows compile issue. Correct me if I'm
wrong, but I think .*_(gtk|views|webui)\.(h|cc) is used much more often than
(gtk|views|webui)_.*\.(h|cc) in the code so I think
edit_search_engine_dialog_webui.h/cc would be a more consistent name.

[wyck, 2011-10-19 13:52:27]

On 2011/10/18 21:26:49, flackr wrote:
> I'm really glad you figured out the Windows compile issue. Correct me if I'm
> wrong, but I think .*_(gtk|views|webui)\.(h|cc) is used much more often than
> (gtk|views|webui)_.*\.(h|cc) in the code so I think
> edit_search_engine_dialog_webui.h/cc would be a more consistent name.

OK.  I'll do that.

[commit-bot: I haz the power, 2011-10-19 14:18:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wyck@chromium.org/8118012/46020

[commit-bot: I haz the power, 2011-10-19 16:11:18]

Commit queue rejected this change because the description was changed
between the time the change entered the commit queue and the time it
was ready to commit. You can safely check the commit box again.

[commit-bot: I haz the power, 2011-10-19 16:53:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wyck@chromium.org/8118012/46020

[commit-bot: I haz the power, 2011-10-19 18:39:02]

Change committed as 106336

[wyck, 2011-10-19 19:08:58]

On 2011/10/19 18:39:02, I haz the power (commit-bot) wrote:
> Change committed as 106336

Qapla'!