cc'ing Ligi for further triage.

Able to reproduce in chrome stable - 59.0.3071.86. 

Precondition: Enable - startup option set for Google Chrome-> Startup.  "users can override " default user option wouldn't work.

Server
=======
1. Launch GPO Management Editor.
2. Google Chrome - Default settings ( users can override)
3. Action on startup - Enabled.

Client
======
1. gpupdate and launch chrome
2. chrome://settings , On Startup

Observed
========
Startup options cannot be modified.

Expected
========
Startup options can be modified.

Will try the same in M58 and update the behavior. 

Note: Not a release blocker if this is confirmed as regression.

Not able to reproduce the issue on chrome stable - 58.0.3029.110

Precondition: Enable - startup option set for Google Chrome-> Startup.  "users can override " default user option wouldn't work.

Server
=======
1. Launch GPO Management Editor.
2. Google Chrome - Default settings (users can override)
3. Action on startup - Enabled.

Client
======
1. gpupdate and launch chrome
2. chrome://settings , On Startup

Observed
========
Startup options can be modified.

Attaching the screen-cast for reference.


Thank You...

Deleted: Issue 732471.mp4 3.3 MB

	Issue 732471.mp4 3.3 MB View Download

Kiran, did you set the Precondition: Enable - startup option set for Google Chrome-> Startup. 
I did not see that in video, without that the bug won't be reproducible.

Reproducible in Chrome 59 and not reproducible in Chrome 58. This is due to the new design implementation of chrome://settings in M59. The option for "set pages" is not available in M59.

Looping to tbuckley@ who is the owner as per launch bug for Material Design Settings page  : Issue 478982

Deleted: MDSetting.jpg 46.8 KB

	MDSetting.jpg 46.8 KB View Download

 Issue 735903  has been merged into this issue.

+dpapad, stevenjb -- any idea what happened here? Seems like we might be making it uneditable whenever there's a policy, even if the policy is that it should be editable.

@dbeam: Do you know how to set this policy locally without involving a server? I recall you were doing this while developing policy related UIs.

Is "GPO Management Editor" the Google Admin console?

In MD Settings there should be "Add a new page" and "Use current pages" buttons when "Open a specific page..." is selected, are those missing? There should also be a menu to edit/remove entries. (i.e. instead of there being a separate UI to "Set pages" it is implemented inline).

Hmm, from the Admin console I only seem to be able to enforce this setting, there is no recommended (Default + users can override) option that I can see. I am assuming there is another way to set this to "recommended" that I am unfamiliar with.

Looking at the code, we appear to just be looking at prefs.session.startup_urls.controlledBy instead of prefs.session.startup_urls.enforcement == chrome.settingsPrivate.Enforcement.ENFORCED:

https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/on_startup_page/startup_urls_page.html?q=startup_urls+package:%5Echromium$&l=1

@ligimole: Can you compare M59 chrome://settings (new) and chrome://settings-frame (old)? FWIW, the old page is still available in M59, and this could be proposed as a workaround to whoever is affected.

In the meantime, I am trying to reprocuce this locally and compare them myself.

So I am able to reproduce this locally, as follows (also see screencast):

1) Paste the following contents to /etc/opt/chrome/policies/managed/test_policy.json (create this file).

{
  "RestoreOnStartup": 4,
  "RestoreOnStartupURLs": ["chrome://policy", "chrome://downloads"]
}

2) Go to chrome://policy and click "reload policies"
3) Go to chrome://settings

What I am noticing is that both the old and new UI have the same behavior of not allowing adding any more startup pages. In other words the old UI's behavior seems to have changed from M58 to M59.

@dbeam,stevenjb: Do you know if this was intentional? I am guessing no. In which case a bisect would be very helpful.

Deleted: startup_pages_policy.mp4 343 KB

	startup_pages_policy.mp4 343 KB View Download

I went as far back as M56, and the "Set pages" button is disabled for me (on Linux), so starting to wonder if the button was only ever enabled on Windows when those policies are effective.

As per comment #12, tested this issue on Windows 10 with chrome #59.0.3071.36

Observed that under "chrome://settings-frame -> onstartup" options, we can add the sites, which are not mentioned in the gpo policy settings.

Attaching the screen-cast for reference. 

Thank You...

Deleted: Issue 732471.mp4 4.0 MB

	Issue 732471.mp4 4.0 MB View Download

+georgesak

@georgesak: Do you know what is the intended behavior when the following policies are set?

"RestoreOnStartup": 4,
"RestoreOnStartupURLs": ["chrome://policy", "chrome://settings"]

Should the user be able to, add more startup URLs besides the ones enforced by policy? See previous video from 0:43 to 1:00. This seems like bug on the old Settings page to me. The fact that the 'x' icon is briefly shown when hovering over policy encorced URLs, adds to my suspicion. FWIW, I have not been able to reproduce this behavior on Linux (per comment#13). Still trying to figure out how to simulate those policies on Windows.  

dpapad@ - FWIW, on Chrome OS some policies may be "recommended" instead of "enforced".

Are we sure that the behavior changed from 58 to 59, or is it possible that the original repro is with a policy that sets RestoreOnStartupURLs to "Recommended"?

@stevenjb, you are right. You mentioned this at comment#11, but did not grasp it until now. So I can reproduce the following locally now:

1) Put the following contents at /etc/chromium/policies/recommended/test_policy.json
{
  "RestoreOnStartup": 4,
  "RestoreOnStartupURLs": ["chrome://md-settings", "chrome://settings-frame"]
}
2) Go to chrome://policy and refresh
3) Compare chrome://md-settings and chrome://settings-frame

In md-settings can't add any new URLs. In settings-frame you can add new URLs. The related code is at [1] and we should also audit other places where we might have accidentally handled "recommended" and "enforced" erroneously.

[1] https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/on_startup_page/startup_urls_page.html?dr=C&l=45

I know at least one other place where I ended up duplicating the (fairly trivial logic) in CrPolicyPrefBehavior,isPrefEnforced().

It occurred to me at the time to re-factor isPrefEnforced to take a |pref| argument instead of assuming that this.pref exists (which is not ideal in a behavior that does not define |pref| anyway), then we could include that behavior and use isPrefEnforced(pref) where we care about enforced vs. recommended.

And I agree that we should do an audit as well.

If you want to assign this to me I have a pretty good idea of what we should do and it shouldn't be super difficult, but my plate is also pretty full if you're feeling up to it yourself, I'm happy to review :)

@stevenjb: I can handle this one if you have a full plate, and maybe you can help with the other places? Still have a question though, because regarding "recommended" startup URLs and "Use current pages" button. 

Currently (in old Settings) the user is able to
 - Overwrite "recommended" URLs by using "Use current pages".
 - Edit/Delete "recommended" URLs.

I suppose we want to preserve this behavior in MD Settings. Can you verify? In other words "recommended" URLs are supposed to be editable just like any other URL?

That sounds right to me. In general I would go with "preserve existing behavior" unless we explicitly decide to do otherwise, and I don't see any particular reason to change this.

Also, generally speaking, "recommended" does sometimes refer to "here are some handy defaults, feel free to change them", which seems consistent with the old settings implementation.

I did a quick audit and can not find any other direct uses of controlledBy or enforcement in the rest of the Settings UI, so I think we are good.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/c9a02feda8372a18c1b2d124448424225148698b

commit c9a02feda8372a18c1b2d124448424225148698b
Author: dpapad <dpapad@chromium.org>
Date: Sat Jun 24 00:06:19 2017

MD Settings: Allow editing startup URLs when "recommended" policy is present.

Before this CL, "recommended" and "managed" startup URL policies were treated
identically.

BUG= 732471 
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2953343002
Cr-Commit-Position: refs/heads/master@{#482082}

[modify] https://crrev.com/c9a02feda8372a18c1b2d124448424225148698b/chrome/browser/resources/settings/on_startup_page/compiled_resources2.gyp
[modify] https://crrev.com/c9a02feda8372a18c1b2d124448424225148698b/chrome/browser/resources/settings/on_startup_page/startup_urls_page.html
[modify] https://crrev.com/c9a02feda8372a18c1b2d124448424225148698b/chrome/browser/resources/settings/on_startup_page/startup_urls_page.js

@tbuckley: Wondering if there is good enough reason to merge to M59/M60, given that in those versions chrome://settings-frame is still available as a workaround.

Please merge request for M60, M59 is already sailed.

Updating title to be more accurate.

This bug requires manual review: M60 has already been promoted to the beta branch, so this requires manual review
Please contact the milestone owner if you have questions.
Owners: amineer@(Android), cmasso@(iOS), josafat@(ChromeOS), bustamante@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Approving merge to M60. 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/0200c8a2a2542eb3f87c32d7f9da5f46b8e71360

commit 0200c8a2a2542eb3f87c32d7f9da5f46b8e71360
Author: dpapad <dpapad@chromium.org>
Date: Mon Jun 26 22:39:14 2017

M60 merge: MD Settings: Allow editing startup URLs when "recommended" policy is present.

Before this CL, "recommended" and "managed" startup URL policies were treated
identically.

BUG= 732471 
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2953343002
Cr-Original-Commit-Position: refs/heads/master@{#482082}
Review-Url: https://codereview.chromium.org/2960773002 .
Cr-Commit-Position: refs/branch-heads/3112@{#469}
Cr-Branched-From: b6460e24cf59f429d69de255538d0fc7a425ccf9-refs/heads/master@{#474897}

[modify] https://crrev.com/0200c8a2a2542eb3f87c32d7f9da5f46b8e71360/chrome/browser/resources/settings/on_startup_page/compiled_resources2.gyp
[modify] https://crrev.com/0200c8a2a2542eb3f87c32d7f9da5f46b8e71360/chrome/browser/resources/settings/on_startup_page/startup_urls_page.html
[modify] https://crrev.com/0200c8a2a2542eb3f87c32d7f9da5f46b8e71360/chrome/browser/resources/settings/on_startup_page/startup_urls_page.js

Verified this issue on Windows 7 with chrome #61.0.3141.0 

These are the steps followed

Server
=======
1. Launch GPO Management Editor.
2. Google Chrome - Default settings (users can override)
3. Action on startup - Enabled.

Client
======
1. gpupdate and launch chrome
2. chrome://settings , On Startup

Observed
========
Startup options can be modified. from "chrome://settings" page

Hence adding TE-Verified labels,attaching the screen-cast for reference.

 
Thank You...

Deleted: 732471.mp4 3.2 MB

	732471.mp4 3.2 MB View Download

Verified this issue on Windows 7 with chrome #60.0.3112.50 

These are the steps followed

Server
=======
1. Launch GPO Management Editor.
2. Google Chrome - Default settings (users can override)
3. Action on startup - Enabled.

Client
======
1. gpupdate and launch chrome
2. chrome://settings , On Startup

Observed
========
Startup options can be modified. from "chrome://settings" page

Hence adding TE-Verified labels, attaching the screen-cast for reference.

 
Thank You...

Deleted: 732471.mp4 5.3 MB

	732471.mp4 5.3 MB View Download

Finally caught up to this, thank you all for fixing this regression.