chrome://settings - Provide method for pages to prevent themselves being shown.

chrome://settings - Provide method for pages to prevent themselves being shown.

use this method in:
- alert overlay
- add startup page overlay
- password manager overlay

these are the places I determined should be sometimes disabled when I did a sweep of the code, but I'm probably missing something.

BUG=none
TEST=chrome://settings/alertOverlay should show the basic browser page

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

[stuartmorgan, 2011-02-12 01:09:05]

http://codereview.chromium.org/6480039/diff/2001/chrome/browser/resources/opt...
File chrome/browser/resources/options/add_startup_page_overlay.js (right):

http://codereview.chromium.org/6480039/diff/2001/chrome/browser/resources/opt...
chrome/browser/resources/options/add_startup_page_overlay.js:117: return
!$('startupAddButton').disabled;
I'm not wild on using UI state for this. How about a "static" method on the
Browser page object that returns whether or not it should be enabled, and base
both this and the UI state updating on that method?

http://codereview.chromium.org/6480039/diff/2001/chrome/browser/resources/opt...
File chrome/browser/resources/options/alert_overlay.js (right):

http://codereview.chromium.org/6480039/diff/2001/chrome/browser/resources/opt...
chrome/browser/resources/options/alert_overlay.js:135: // Intentionally don't
show the URL in the location bar  as we don't want
Fix the double space.

http://codereview.chromium.org/6480039/diff/2001/chrome/browser/resources/opt...
File chrome/browser/resources/options/options_page.js (right):

http://codereview.chromium.org/6480039/diff/2001/chrome/browser/resources/opt...
chrome/browser/resources/options/options_page.js:79: if
(this.showOverlay_(pageName)) {
Change to |if (!targetPage && this.showOverlay_(pageName))|? Seems weird to call
showOverlay_ when we already know it was a page that we couldn't show.

http://codereview.chromium.org/6480039/diff/2001/chrome/browser/resources/opt...
chrome/browser/resources/options/options_page.js:88: pageName = targetPage.name;
Setting paramaters generally makes code harder to understand. Use a new variable
instead, and have everything after use that.

http://codereview.chromium.org/6480039/diff/2001/chrome/browser/resources/opt...
File chrome/browser/resources/options/password_manager.js (right):

http://codereview.chromium.org/6480039/diff/2001/chrome/browser/resources/opt...
chrome/browser/resources/options/password_manager.js:54: return
!$('manage-passwords').disabled;
As with the other case, use a function that gives an answer, not the UI state.

[Evan Stade, 2011-02-12 03:06:42]

http://codereview.chromium.org/6480039/diff/2001/chrome/browser/resources/opt...
File chrome/browser/resources/options/add_startup_page_overlay.js (right):

http://codereview.chromium.org/6480039/diff/2001/chrome/browser/resources/opt...
chrome/browser/resources/options/add_startup_page_overlay.js:117: return
!$('startupAddButton').disabled;
On 2011/02/12 01:09:06, stuartmorgan wrote:
> I'm not wild on using UI state for this. How about a "static" method on the
> Browser page object that returns whether or not it should be enabled, and base
> both this and the UI state updating on that method?

There is already such a function, called
shouldEnableCustomStartupPageControls_(), and it also relies on "UI state". I
will use it here.

The scare quotes there indicate that I don't think this is UI state. I believe
this attribute is logically meaningful. The details of how it is represented in
the UI are abstracted (it happens to be greyed out, but that could be changed
with css right?). My point being, I believe that
shouldEnableCustomStartupPageControls_ is ok, even though it seems to contradict
the spirit of your comment.

http://codereview.chromium.org/6480039/diff/2001/chrome/browser/resources/opt...
File chrome/browser/resources/options/alert_overlay.js (right):

http://codereview.chromium.org/6480039/diff/2001/chrome/browser/resources/opt...
chrome/browser/resources/options/alert_overlay.js:135: // Intentionally don't
show the URL in the location bar  as we don't want
On 2011/02/12 01:09:06, stuartmorgan wrote:
> Fix the double space.

Done.

http://codereview.chromium.org/6480039/diff/2001/chrome/browser/resources/opt...
File chrome/browser/resources/options/options_page.js (right):

http://codereview.chromium.org/6480039/diff/2001/chrome/browser/resources/opt...
chrome/browser/resources/options/options_page.js:79: if
(this.showOverlay_(pageName)) {
On 2011/02/12 01:09:06, stuartmorgan wrote:
> Change to |if (!targetPage && this.showOverlay_(pageName))|? Seems weird to
call
> showOverlay_ when we already know it was a page that we couldn't show.

Done.

http://codereview.chromium.org/6480039/diff/2001/chrome/browser/resources/opt...
chrome/browser/resources/options/options_page.js:88: pageName = targetPage.name;
On 2011/02/12 01:09:06, stuartmorgan wrote:
> Setting paramaters generally makes code harder to understand. Use a new
variable
> instead, and have everything after use that.

on the other hand, I don't want someone coming in and adding new code that uses
pageName because they don't realize it's no longer valid (this is a rather long
function). Most likely if they do this, their code will still appear to work
because it's only in unusual cases that pageName is invalidated.

http://codereview.chromium.org/6480039/diff/2001/chrome/browser/resources/opt...
File chrome/browser/resources/options/password_manager.js (right):

http://codereview.chromium.org/6480039/diff/2001/chrome/browser/resources/opt...
chrome/browser/resources/options/password_manager.js:54: return
!$('manage-passwords').disabled;
On 2011/02/12 01:09:06, stuartmorgan wrote:
> As with the other case, use a function that gives an answer, not the UI state.

Done.

[stuartmorgan, 2011-02-14 18:50:16]

LGTM

[stuartmorgan, 2011-02-14 18:54:58]

On 2011/02/14 18:50:16, stuartmorgan wrote:
> LGTM

Whoops, wrong CL. I'm just starting on this one; sorry.

[stuartmorgan, 2011-02-14 18:59:35]

LGTM (for realz this time)

[Vitaly Pavlenko, 2014-10-01 23:29:51]

vitalyp@chromium.org changed reviewers:
+ vitalyp@chromium.org

[Vitaly Pavlenko, 2014-10-01 23:29:53]

https://codereview.chromium.org/6480039/diff/2004/chrome/browser/resources/op...
File chrome/browser/resources/options/alert_overlay.js (right):

https://codereview.chromium.org/6480039/diff/2004/chrome/browser/resources/op...
chrome/browser/resources/options/alert_overlay.js:75: canShow_ = false;
This should be 

this.canShow_ = false;

right?

[Evan Stade, 2014-10-13 16:21:55]

https://codereview.chromium.org/6480039/diff/2004/chrome/browser/resources/op...
File chrome/browser/resources/options/alert_overlay.js (right):

https://codereview.chromium.org/6480039/diff/2004/chrome/browser/resources/op...
chrome/browser/resources/options/alert_overlay.js:75: canShow_ = false;
On 2014/10/01 23:29:53, Vitaly Pavlenko wrote:
> This should be 
> 
> this.canShow_ = false;
> 
> right?

yes