Options: Fix to enable "Use current pages" to make new URLs to open when chrome restarts.

Options: Fix to enable "Use current pages" to make new URLs to open when chrome restarts.

Made changes to enable the button only on below conditions
1) If pref is user editable. In this case event.value['disabled'] would be false.
2) If pref is user editable and it does'nt hold a default value. In this case event.value['controlledBy'] would be recommended.
3) Otherwise if controlledBy is empty or null.

BUG=106592
TEST=None.


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

[NaveenBobbili (Motorola), 2011-12-08 14:19:30]

Please review.

[James Hawkins, 2011-12-08 18:11:23]

I don't see how this fixes the linked bug.

[NaveenBobbili (Motorola), 2011-12-09 01:32:02]

On 2011/12/08 18:11:23, James Hawkins wrote:
> I don't see how this fixes the linked bug.

Below is the scenario when "Open following Pages " radio button is selected.
1) When chrome is installed for the first time
shouldEnableCustomStartupPageControls returns true as
this.startup_pages_pref_.controlledBy = Undefined. So, "Use current Pages" is
enabled.
2) In further executions shouldEnableCustomStartupPageControls returns false as
this.startup_pages_pref_.controlledBy = recommended. Thats the reason why "Use
current Pages" is never enabled again.

So, from my changes shouldEnableCustomStartupPageControls  would return true and
thereby make the "Use current Pages" button enabled when pref is editable i.e
this.startup_pages_pref_.controlledBy = recommended and also
this.startup_pages_pref_.disabled = false.

Please let me know if I am missing something.

[James Hawkins, 2011-12-10 04:28:21]

+joaodasilva

[Joao da Silva, 2011-12-10 14:53:30]

Thanks for CCing me on this one. There are actually two issues here:

1) Prefs UI code shouldn't check the "controlledBy" attribute (that's meant for
other purposes), but the "disabled" attribute instead. That attribute does The
Right Thing regarding policies (mandatory or recommended level), extensions,
etc. I've verified this on a local build with these changes, and all is well
with a bunch of different policy settings for the startup pages.

2) controlledBy should not be defined, but will always be "recommended" for
every pref customized by the user. This _shouldn't_ affect anything anyway
(since code should check "disabled", not "controlledBy"...), and is a different
bug by itself. I'll open a bug for that after figuring where that was
introduced.

Please do the changes mentioned inline, and land this only if James and Bernhard
agree. Thanks!

http://codereview.chromium.org/8883015/diff/1/chrome/browser/resources/option...
File chrome/browser/resources/options/browser_options.js (right):

http://codereview.chromium.org/8883015/diff/1/chrome/browser/resources/option...
chrome/browser/resources/options/browser_options.js:252:
!this.startup_pages_pref_.disabled;
Remove the "controlledBy" lines. It suffices to do this:

return $('startupShowPagesButton').checked &&
    !this.startup_pages_pref_.disabled;

http://codereview.chromium.org/8883015/diff/1/chrome/browser/resources/option...
chrome/browser/resources/options/browser_options.js:292:
this.startup_pages_pref_.controlledBy = event.value['controlledBy'];
Remove the ".controlledBy" line, it's not needed.

[NaveenBobbili (Motorola), 2011-12-10 17:36:14]

Sure. Even I felt that there is more need to check 'disabled' attribute rather
,since it does the right check in CoreOptionsHandler::CreateValueForPref(). But
since I didn't know the exact reason for using 'controlledBy' , I left the
conditional statement without disturbing. Now I have removed controlledBy
checking after your comments. Please take a look now.

http://codereview.chromium.org/8883015/diff/1/chrome/browser/resources/option...
File chrome/browser/resources/options/browser_options.js (right):

http://codereview.chromium.org/8883015/diff/1/chrome/browser/resources/option...
chrome/browser/resources/options/browser_options.js:252:
!this.startup_pages_pref_.disabled;
On 2011/12/10 14:53:30, Joao da Silva wrote:
> Remove the "controlledBy" lines. It suffices to do this:
> 
> return $('startupShowPagesButton').checked &&
>     !this.startup_pages_pref_.disabled;

Done.

http://codereview.chromium.org/8883015/diff/1/chrome/browser/resources/option...
chrome/browser/resources/options/browser_options.js:292:
this.startup_pages_pref_.controlledBy = event.value['controlledBy'];
On 2011/12/10 14:53:30, Joao da Silva wrote:
> Remove the ".controlledBy" line, it's not needed.

Done.

[Joao da Silva, 2011-12-10 17:41:41]

LGTM, but please let James and Bernhard have a saying too. Thanks for fixing
this!

[James Hawkins, 2011-12-10 17:44:55]

lgtm

[NaveenBobbili (Motorola), 2011-12-11 03:39:06]

On 2011/12/10 17:44:55, James Hawkins wrote:
> lgtm

I am committing this change. I will also check the reason for incorporating
controlledBy previously and then fix it if there is a bug.

[commit-bot: I haz the power, 2011-12-11 03:39:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/qghc36@motorola.com/8883015/5003

[Bernhard Bauer, 2011-12-11 04:30:36]

LGTM

http://codereview.chromium.org/8883015/diff/5003/chrome/browser/resources/opt...
File chrome/browser/resources/options/browser_options.js (right):

http://codereview.chromium.org/8883015/diff/5003/chrome/browser/resources/opt...
chrome/browser/resources/options/browser_options.js:27: 'managed': false
If someone feels like yak shaving, this got left behind when renaming "managed"
to "controlledBy", so it should be renamed to "disabled" now :)

[NaveenBobbili (Motorola), 2011-12-11 12:42:11]

On 2011/12/11 04:30:36, Bernhard Bauer wrote:
> LGTM
> 
>
http://codereview.chromium.org/8883015/diff/5003/chrome/browser/resources/opt...
> File chrome/browser/resources/options/browser_options.js (right):
> 
>
http://codereview.chromium.org/8883015/diff/5003/chrome/browser/resources/opt...
> chrome/browser/resources/options/browser_options.js:27: 'managed': false
> If someone feels like yak shaving, this got left behind when renaming
"managed"
> to "controlledBy", so it should be renamed to "disabled" now :)

Done

[commit-bot: I haz the power, 2011-12-11 12:42:34]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/qghc36@motorola.com/8883015/11001

[commit-bot: I haz the power, 2011-12-12 03:00:18]

Change committed as 113975