Issue 3171003: DOMUI: Add basic HTML for the AutoFill options page.

Issue 3171003: DOMUI: Add basic HTML for the AutoFill options page. (Closed)

Created:
10 years, 4 months ago by James Hawkins

Modified:
9 years, 7 months ago

Reviewers:
stuartmorgan, arv (Not doing code reviews), zel

CC:
chromium-reviews, dhg, ben+cc_chromium.org, arv (Not doing code reviews), dhollowa

Visibility:
Public.

Description

DOMUI: Add basic HTML for the AutoFill options page. BUG=49094 TEST=none Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=55957

Patch Set 3 : 80 cols. #

Created: 10 years, 4 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+63 lines, -0 lines)			Patch
M	chrome/browser/dom_ui/autofill_options_handler.cc	View		1 chunk	+16 lines, -0 lines	0 comments	Download
M	chrome/browser/resources/options.html	View		1 chunk	+1 line, -0 lines	0 comments	Download
M	chrome/browser/resources/options/autofill_options.html	View	1 2	1 chunk	+31 lines, -0 lines	0 comments	Download
A	chrome/browser/resources/options/autofill_options_page.css	View		1 chunk	+15 lines, -0 lines	0 comments	Download

Messages

Total messages: 7 (0 generated)

Expand Messages | Collapse Messages

arv (Not doing code reviews)

http://codereview.chromium.org/3171003/diff/1/3 File chrome/browser/resources/options/autofill_options.html (right): http://codereview.chromium.org/3171003/diff/1/3#newcode1 chrome/browser/resources/options/autofill_options.html:1: <head> <head> is not valid here http://codereview.chromium.org/3171003/diff/1/3#newcode2 chrome/browser/resources/options/autofill_options.html:2: <style> ...

10 years, 4 months ago (2010-08-12 00:42:40 UTC) #2

James Hawkins

10 years, 4 months ago (2010-08-12 20:06:26 UTC) #3

http://codereview.chromium.org/3171003/diff/1/3
File chrome/browser/resources/options/autofill_options.html (right):

http://codereview.chromium.org/3171003/diff/1/3#newcode1
chrome/browser/resources/options/autofill_options.html:1: <head>
On 2010/08/12 00:42:40, arv wrote:
> <head> is not valid here

Done.

http://codereview.chromium.org/3171003/diff/1/3#newcode2
chrome/browser/resources/options/autofill_options.html:2: <style>
On 2010/08/12 00:42:40, arv wrote:
> Move this to a css file

Done.

http://codereview.chromium.org/3171003/diff/1/3#newcode19
chrome/browser/resources/options/autofill_options.html:19: </h1><br>
On 2010/08/12 00:42:40, arv wrote:
> Do not use <br> (unless for linebreaking text)
> 
> Use display block and margins

Done.

http://codereview.chromium.org/3171003/diff/1/3#newcode28
chrome/browser/resources/options/autofill_options.html:28: <select
id="profileList" "size=10" multiple="multiple">
On 2010/08/12 00:42:40, arv wrote:
> size="10"

Done.

http://codereview.chromium.org/3171003/diff/1/3#newcode28
chrome/browser/resources/options/autofill_options.html:28: <select
id="profileList" "size=10" multiple="multiple">
On 2010/08/12 00:42:40, arv wrote:
> multiple does not take a value
> 
> <select id="..." size="10" multiple>

Done.

http://codereview.chromium.org/3171003/diff/1/3#newcode29
chrome/browser/resources/options/autofill_options.html:29: <option
value="addressesHeader" disabled="disabled"
i18n-content="addressesHeader"></option>
On 2010/08/12 00:42:40, arv wrote:
> disabled does not take a value
> 
> <option value="..." disabled>

Done.

http://codereview.chromium.org/3171003/diff/1/3#newcode36
chrome/browser/resources/options/autofill_options.html:36: <button
id="addAddressButton" i18n-content="addAddressButton"></button><br>
On 2010/08/12 00:42:40, arv wrote:
> Remove br. Wrap in div or set display to block

Done.

http://codereview.chromium.org/3171003/diff/1/3#newcode43
chrome/browser/resources/options/autofill_options.html:43: <a
href="http://www.google.com/support/chrome/bin/answer.py?answer=142893"
target=”_blank” i18n-content="helpButton"></a>
On 2010/08/12 00:42:40, arv wrote:
> long line

Done.

zel

LGTM + few nits http://codereview.chromium.org/3171003/diff/5001/6003 File chrome/browser/resources/options/autofill_options.html (right): http://codereview.chromium.org/3171003/diff/5001/6003#newcode16 chrome/browser/resources/options/autofill_options.html:16: <option value="addressesHeader" disabled i18n-content="addressesHeader"></option> 80 ...

10 years, 4 months ago (2010-08-12 21:15:13 UTC) #5

James Hawkins

http://codereview.chromium.org/3171003/diff/5001/6003 File chrome/browser/resources/options/autofill_options.html (right): http://codereview.chromium.org/3171003/diff/5001/6003#newcode16 chrome/browser/resources/options/autofill_options.html:16: <option value="addressesHeader" disabled i18n-content="addressesHeader"></option> On 2010/08/12 21:15:13, zel wrote: ...

10 years, 4 months ago (2010-08-12 21:33:11 UTC) #6

LGTM

Expand Messages | Collapse Messages