Issue 2830036: Advanced (Under the Hood) options panel code review

Issue 2830036: Advanced (Under the Hood) options panel code review (Closed)

Created:
10 years, 5 months ago by csilv

Modified:
9 years, 7 months ago

Reviewers:
dhg, arv (Not doing code reviews), James Hawkins, zel, Nico

CC:
chromium-reviews, ben+cc_chromium.org, stuartmorgan, Evan Stade, sargrass

Base URL:
svn://svn.chromium.org/chrome/trunk/src/

Visibility:
Public.

Description

- First round implementation of the Advanced (Under the Hood) options panel. - All checkboxes are functional, partial functionality for buttons. - Download path controls TBD. - Security SSL checkboxes on non-Mac platform TBD. - Added placeholder files for Browser and Personal option panels. - Moved placeholder CSS into a new options_page.css file. - Unit testing TBD. BUG=48482 TEST=Launch browser with --enable-tabbed-options and bring up options window. Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=51770

Patch Set 1 #

Total comments: 27

Patch Set 2 : '' #

Total comments: 15

Patch Set 3 : '' #

Total comments: 6

Patch Set 4 : '' #

Patch Set 5 : '' #

Patch Set 6 : '' #

Patch Set 7 : '' #

Created: 10 years, 5 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+775 lines, -91 lines)			Patch
A	chrome/browser/dom_ui/advanced_options_handler.h	View	1 2 3	1 chunk	+40 lines, -0 lines	0 comments	Download
A	chrome/browser/dom_ui/advanced_options_handler.cc	View	1 2	1 chunk	+123 lines, -0 lines	0 comments	Download
A	chrome/browser/dom_ui/advanced_options_utils_mac.h	View	1 2	1 chunk	+23 lines, -0 lines	0 comments	Download
A	chrome/browser/dom_ui/advanced_options_utils_mac.mm	View	1 2	1 chunk	+40 lines, -0 lines	0 comments	Download
A	chrome/browser/dom_ui/browser_options_handler.h	View	1 2	1 chunk	+23 lines, -0 lines	0 comments	Download
A	chrome/browser/dom_ui/browser_options_handler.cc	View		1 chunk	+68 lines, -0 lines	0 comments	Download
M	chrome/browser/dom_ui/core_options_handler.cc	View	1 2 3 4 5 6	1 chunk	+6 lines, -7 lines	0 comments	Download
M	chrome/browser/dom_ui/options_ui.cc	View	1 2 3 4 5 6	2 chunks	+6 lines, -0 lines	0 comments	Download
A	chrome/browser/dom_ui/personal_options_handler.h	View	1 2	1 chunk	+23 lines, -0 lines	0 comments	Download
A	chrome/browser/dom_ui/personal_options_handler.cc	View		1 chunk	+60 lines, -0 lines	0 comments	Download
M	chrome/browser/resources/options.html	View	1 2 3 4 5 6	5 chunks	+24 lines, -75 lines	0 comments	Download
A	chrome/browser/resources/options/advanced_options.html	View	1 2 3	1 chunk	+128 lines, -0 lines	0 comments	Download
A	chrome/browser/resources/options/advanced_options.js	View	1 2	1 chunk	+66 lines, -0 lines	0 comments	Download
A	chrome/browser/resources/options/browser_options.html	View	1 2	1 chunk	+3 lines, -0 lines	0 comments	Download
A	chrome/browser/resources/options/browser_options.js	View		1 chunk	+32 lines, -0 lines	0 comments	Download
M	chrome/browser/resources/options/chromeos_system_options.html	View	3 4 5 6	4 chunks	+10 lines, -9 lines	0 comments	Download
A	chrome/browser/resources/options/options_page.css	View	1 2	1 chunk	+57 lines, -0 lines	0 comments	Download
A	chrome/browser/resources/options/personal_options.html	View	1 2	1 chunk	+3 lines, -0 lines	0 comments	Download
A	chrome/browser/resources/options/personal_options.js	View		1 chunk	+32 lines, -0 lines	0 comments	Download
M	chrome/chrome_browser.gypi	View	1 2 3 4 5 6	1 chunk	+8 lines, -0 lines	0 comments	Download

Messages

Total messages: 15 (0 generated)

Expand Messages | Collapse Messages

csilv

This is an implementation of the Advanced (Under the Hood) options panel. While the panel ...

10 years, 5 months ago (2010-07-01 22:02:28 UTC) #1

arv (Not doing code reviews)

Awesome http://codereview.chromium.org/2830036/diff/1/3 File chrome/browser/dom_ui/advanced_options_handler.cc (right): http://codereview.chromium.org/2830036/diff/1/3#newcode26 chrome/browser/dom_ui/advanced_options_handler.cc:26: std::wstring(L"<a href='") + You should not need to ...

10 years, 5 months ago (2010-07-01 22:25:38 UTC) #2

zel

http://codereview.chromium.org/2830036/diff/12001/13002 File chrome/browser/dom_ui/advanced_options_handler.cc (right): http://codereview.chromium.org/2830036/diff/12001/13002#newcode25 chrome/browser/dom_ui/advanced_options_handler.cc:25: localized_strings->SetString(L"privacyLearnMore", I believe that you should be able to ...

10 years, 5 months ago (2010-07-01 22:41:59 UTC) #3

zel

http://codereview.chromium.org/2830036/diff/12001/13020 File chrome/browser/resources/options_chromeos.html (right): http://codereview.chromium.org/2830036/diff/12001/13020#newcode1 chrome/browser/resources/options_chromeos.html:1: <!DOCTYPE HTML> On 2010/07/01 22:41:59, zel wrote: > we ...

10 years, 5 months ago (2010-07-01 22:43:11 UTC) #4

James Hawkins

http://codereview.chromium.org/2830036/diff/1/4 File chrome/browser/dom_ui/advanced_options_handler.h (right): http://codereview.chromium.org/2830036/diff/1/4#newcode8 chrome/browser/dom_ui/advanced_options_handler.h:8: #include <string> Looks like you don't need this include. ...

10 years, 5 months ago (2010-07-01 22:44:02 UTC) #5

dhg

http://codereview.chromium.org/2830036/diff/12001/13013 File chrome/browser/resources/options/advanced_options.html (right): http://codereview.chromium.org/2830036/diff/12001/13013#newcode3 chrome/browser/resources/options/advanced_options.html:3: <div class="header-title" i18n-content="advancedPage">UNDER THE HOOD</div> That goes for the ...

10 years, 5 months ago (2010-07-01 22:59:09 UTC) #6

csilv

Thank you all for thorough reviews. I believe I've addressed all the feedback, please give ...

10 years, 5 months ago (2010-07-02 02:05:37 UTC) #7

Thank you all for thorough reviews.  I believe I've addressed all the feedback,
please give another review.  thanks!

http://codereview.chromium.org/2830036/diff/1/3
File chrome/browser/dom_ui/advanced_options_handler.cc (right):

http://codereview.chromium.org/2830036/diff/1/3#newcode26
chrome/browser/dom_ui/advanced_options_handler.cc:26: std::wstring(L"<a href='")
+
On 2010/07/01 22:25:38, arv wrote:
> You should not need to put HTML in here.

Done.

http://codereview.chromium.org/2830036/diff/1/4
File chrome/browser/dom_ui/advanced_options_handler.h (right):

http://codereview.chromium.org/2830036/diff/1/4#newcode8
chrome/browser/dom_ui/advanced_options_handler.h:8: #include <string>
On 2010/07/01 22:44:02, James Hawkins wrote:
> Looks like you don't need this include.

Done.

http://codereview.chromium.org/2830036/diff/1/4#newcode9
chrome/browser/dom_ui/advanced_options_handler.h:9: #include <vector>
On 2010/07/01 22:44:02, James Hawkins wrote:
> Nor this one.

Done.

http://codereview.chromium.org/2830036/diff/1/5
File chrome/browser/dom_ui/advanced_options_utils_mac.h (right):

http://codereview.chromium.org/2830036/diff/1/5#newcode17
chrome/browser/dom_ui/advanced_options_utils_mac.h:17: void
ShowNetworkProxySettings();
On 2010/07/01 22:44:02, James Hawkins wrote:
> ShowNetworkProxySettings() and ShowManagerSSLCertificates() should be static.

Done.

http://codereview.chromium.org/2830036/diff/1/5#newcode23
chrome/browser/dom_ui/advanced_options_utils_mac.h:23:
DISALLOW_COPY_AND_ASSIGN(AdvancedOptionsUtilities);
On 2010/07/01 22:44:02, James Hawkins wrote:
> If this class is a utility class, it probably shouldn't store data and should
> use DISALLOW_IMPLICIT_CONSTRUCTORS here.

Done.

http://codereview.chromium.org/2830036/diff/1/8
File chrome/browser/dom_ui/browser_options_handler.h (right):

http://codereview.chromium.org/2830036/diff/1/8#newcode8
chrome/browser/dom_ui/browser_options_handler.h:8: #include <string>
On 2010/07/01 22:44:02, James Hawkins wrote:
> No need for these includes.

Done.

http://codereview.chromium.org/2830036/diff/1/12
File chrome/browser/dom_ui/personal_options_handler.h (right):

http://codereview.chromium.org/2830036/diff/1/12#newcode8
chrome/browser/dom_ui/personal_options_handler.h:8: #include <string>
On 2010/07/01 22:44:02, James Hawkins wrote:
> No need for these includes.

Done.

http://codereview.chromium.org/2830036/diff/1/14
File chrome/browser/resources/options/advanced_options.html (right):

http://codereview.chromium.org/2830036/diff/1/14#newcode5
chrome/browser/resources/options/advanced_options.html:5: <div class="section">
On 2010/07/01 22:25:38, arv wrote:
> HTML5 has a section tag which seems suitable for this.

Done.

http://codereview.chromium.org/2830036/diff/1/14#newcode6
chrome/browser/resources/options/advanced_options.html:6: <div
class="section-title"
On 2010/07/01 22:25:38, arv wrote:
> Can you use a Hx tag since it is clearly a heading?

Done.

http://codereview.chromium.org/2830036/diff/1/15
File chrome/browser/resources/options/advanced_options.js (right):

http://codereview.chromium.org/2830036/diff/1/15#newcode65
chrome/browser/resources/options/advanced_options.js:65: // JSTemplate always
inserts the strings as text, which is usually a
On 2010/07/01 22:25:38, arv wrote:
> Both JSTemplate and i18nTemplate can set innerHTML.
> 
> <foo jsvalues="attribute:value; .domProperty:value"
>           i18n-values="attribute: key; .domProperty: key">
> 
>
https://codesearch.corp.google.com/p#chrome/trunk/src/chrome/tools/test/refer...

Done.

http://codereview.chromium.org/2830036/diff/1/15#newcode69
chrome/browser/resources/options/advanced_options.js:69: elm.innerHTML =
elm.textContent;
On 2010/07/01 22:25:38, arv wrote:
> ... but do you really need to set HTML?

Done.

http://codereview.chromium.org/2830036/diff/1/21
File chrome/browser/resources/options_chromeos.html (right):

http://codereview.chromium.org/2830036/diff/1/21#newcode48
chrome/browser/resources/options_chromeos.html:48: <body onload="load();"
On 2010/07/01 22:25:38, arv wrote:
> Don't use window onload here. Use DOMContentLoaded so we do not have to wait
for
> images etc
> 
> document.addEventListener('DOMContentLoaded', load);

Done.

http://codereview.chromium.org/2830036/diff/1/21#newcode56
chrome/browser/resources/options_chromeos.html:56: <hr/>
On 2010/07/01 22:25:38, arv wrote:
> Invalid HTML

Done. (change made in options.html)

http://codereview.chromium.org/2830036/diff/12001/13002
File chrome/browser/dom_ui/advanced_options_handler.cc (right):

http://codereview.chromium.org/2830036/diff/12001/13002#newcode25
chrome/browser/dom_ui/advanced_options_handler.cc:25:
localized_strings->SetString(L"privacyLearnMore",
On 2010/07/01 22:41:59, zel wrote:
> I believe that you should be able to do all the in HTML  template with
something
> like:
> 
> <a i18n-content="learnMoreLabel" i18n-values=".href:learnMoreUrl"
> target='_blank'></a>

Done.

http://codereview.chromium.org/2830036/diff/12001/13002#newcode107
chrome/browser/dom_ui/advanced_options_handler.cc:107:
dom_ui_->RegisterMessageCallback("showMacNetworkProxySettings",
On 2010/07/01 22:41:59, zel wrote:
> Why are these named as *Mac*.

Done.

http://codereview.chromium.org/2830036/diff/12001/13013
File chrome/browser/resources/options/advanced_options.html (right):

http://codereview.chromium.org/2830036/diff/12001/13013#newcode3
chrome/browser/resources/options/advanced_options.html:3: <div
class="header-title" i18n-content="advancedPage">UNDER THE HOOD</div>
On 2010/07/01 22:41:59, zel wrote:
> do we need placeholder text like "UNDER THE HOOD" in this file?
> isn't this just bloating our executables?

Done.

http://codereview.chromium.org/2830036/diff/12001/13013#newcode3
chrome/browser/resources/options/advanced_options.html:3: <div
class="header-title" i18n-content="advancedPage">UNDER THE HOOD</div>
On 2010/07/01 22:59:09, dhg wrote:
> That goes for the rest of the strings here. Its better to not have anything,
so
> it results in an undefined. Makes it easier to find.
> 
> On 2010/07/01 22:41:59, zel wrote:
> > do we need placeholder text like "UNDER THE HOOD" in this file?
> > isn't this just bloating our executables?
> 
> 

Done.

http://codereview.chromium.org/2830036/diff/12001/13014
File chrome/browser/resources/options/advanced_options.js (right):

http://codereview.chromium.org/2830036/diff/12001/13014#newcode62
chrome/browser/resources/options/advanced_options.js:62:
document.getElementById('tabsToLinksCheckbox').style.display = 'none';
On 2010/07/01 22:41:59, zel wrote:
> use $'tabsToLinksCheckbox') instead of
> document.getElementById('tabsToLinksCheckbox')

Done.

http://codereview.chromium.org/2830036/diff/12001/13014#newcode67
chrome/browser/resources/options/advanced_options.js:67: var elm =
document.getElementById('privacyLearnMore');
On 2010/07/01 22:41:59, zel wrote:
> use $('privacyLearnMore')

This was removed per arv's review.  Obsolete.

http://codereview.chromium.org/2830036/diff/12001/13020
File chrome/browser/resources/options_chromeos.html (right):

http://codereview.chromium.org/2830036/diff/12001/13020#newcode1
chrome/browser/resources/options_chromeos.html:1: <!DOCTYPE HTML>
On 2010/07/01 22:41:59, zel wrote:
> we don't want to have two sets of files for this. instead, i will change grit
> scripts to be able to say something like
> 
> <script ifdef="os_chromeos" src="options/chromeos_system_options.js"></script>
> 
> for now, I suggest that we keep all pages in the same place

Okay, sounds good.  I've reverted the separate version.  We should also try to
omit the HTML source. I assume the grit change will allow for that as well?

Nico

Please open a bug for this (if there isn't one already) and reference it in ...

10 years, 5 months ago (2010-07-02 15:18:00 UTC) #8

zel

Good point. We should file bugs for all other DOM UI options work items. On ...

10 years, 5 months ago (2010-07-02 15:25:42 UTC) #9

zel

LGTM with few minor comments http://codereview.chromium.org/2830036/diff/29001/26003 File chrome/browser/dom_ui/advanced_options_handler.h (right): http://codereview.chromium.org/2830036/diff/29001/26003#newcode28 chrome/browser/dom_ui/advanced_options_handler.h:28: // Callback for the ...

10 years, 5 months ago (2010-07-02 22:34:11 UTC) #10

csilv

http://codereview.chromium.org/2830036/diff/29001/26003 File chrome/browser/dom_ui/advanced_options_handler.h (right): http://codereview.chromium.org/2830036/diff/29001/26003#newcode28 chrome/browser/dom_ui/advanced_options_handler.h:28: // Callback for the "showMacNetworkProxySettings" message. This will invoke ...

10 years, 5 months ago (2010-07-03 00:19:37 UTC) #11

arv (Not doing code reviews)

> http://codereview.chromium.org/2830036/diff/29001/26012#newcode67 > chrome/browser/resources/options.html:67: <hr /> > On 2010/07/02 22:34:11, zel wrote: >> >> remove ...

10 years, 5 months ago (2010-07-03 00:27:48 UTC) #12

csilv

On 2010/07/03 00:27:48, arv wrote: > HTML has no />. It should be <hr> Done.

10 years, 5 months ago (2010-07-03 00:31:20 UTC) #13

csilv

FYI, I'm aware that the unit tests currently fail. This will be resolved once Zel's ...

10 years, 5 months ago (2010-07-03 00:33:23 UTC) #14

zel

10 years, 5 months ago (2010-07-03 00:47:27 UTC) #15

My CL had landed:
http://src.chromium.org/viewvc/chrome?view=rev&revision=51578


On Fri, Jul 2, 2010 at 5:33 PM, <csilv@chromium.org> wrote:

> FYI, I'm aware that the unit tests currently fail.  This will be resolved
> once
> Zel's related CL for conditional includes is submitted.  I will re-base to
> that
> CL and make sure unit tests pass before submitting.
>
>
> http://codereview.chromium.org/2830036/show
>

Expand Messages | Collapse Messages