Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2029263002:
[MD Settings] Add feature to enable md-settings by default. (Closed)
|
Created:
4 years, 6 months ago by groby-ooo-7-16 Modified:
4 years, 6 months ago CC:
chromium-reviews, michaelpg+watch-md-settings_chromium.org, dbeam+watch-settings_chromium.org, stevenjb+watch-md-settings_chromium.org, asvitkine+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[MD Settings] Add feature to enable md-settings by default.
chrome://settings will serve either old settings or md-settings,
depending on flag. chrome://settings-frame will always serve old
settings. chrome://md-settings will serve new settings, unless
the flag is off.
BUG=614758
Committed: https://crrev.com/12483f26c404123844df38d8e8083c1d6dde1426
Cr-Commit-Position: refs/heads/master@{#399640}
Patch Set 1 #
Total comments: 14
Patch Set 2 : Review fixes. #
Total comments: 6
Patch Set 3 : Review fixes. #Patch Set 4 : Moar review fixes. #
Total comments: 7
Patch Set 5 : One last fix... #
Total comments: 2
Patch Set 6 : Rebase to HEAD #Patch Set 7 : git rebase --for-real-no-I-mean-it HEAD #Patch Set 8 : Removing unneeded file. #Patch Set 9 : Fix URL CHECK statement. (Use URL, not Host) #Messages
Total messages: 57 (17 generated)
Description was changed from ========== [MD Settings] Add feature to enable md-settings by default. chrome://settings will serve either old settings or md-settings, depending on flag. chrome://settings-frame will always serve old settings. chrome://md-settings will serve new settings, unless the flag is off. BUG=614758 ========== to ========== [MD Settings] Add feature to enable md-settings by default. chrome://settings will serve either old settings or md-settings, depending on flag. chrome://settings-frame will always serve old settings. chrome://md-settings will serve new settings, unless the flag is off. BUG=614758 ==========
groby@chromium.org changed reviewers: + dbeam@chromium.org
Here's the other approach to do this - I'll close https://codereview.chromium.org/2027813003/ https://codereview.chromium.org/2029263002/diff/1/chrome/browser/ui/webui/set... File chrome/browser/ui/webui/settings/md_settings_ui.cc (right): https://codereview.chromium.org/2029263002/diff/1/chrome/browser/ui/webui/set... chrome/browser/ui/webui/settings/md_settings_ui.cc:85: std::string html_host = chrome::kChromeUIMdSettingsHost; Ugly, but necessary - since we use relative URLs, we'll need to root this at chrome://settings if we serve from chrome://settings So, the question is - do we still want to have chrome://md-settings around when we enable MD from chrome://settings? The CL currently is halfway between. My inclination is to say "no, let's just serve from one URL", but that's not quite what we discussed on the previous CL. LMK
fun fact: you also need to shoehorn in the new MD user manager on this flag as well! luck you! https://codereview.chromium.org/2029263002/diff/1/chrome/browser/ui/webui/set... File chrome/browser/ui/webui/settings/md_settings_ui.cc (right): https://codereview.chromium.org/2029263002/diff/1/chrome/browser/ui/webui/set... chrome/browser/ui/webui/settings/md_settings_ui.cc:89: content::WebUIDataSource::Create(html_host); instead of |html_host|, could this web web_ui->GetURL().host()? https://codereview.chromium.org/2029263002/diff/1/chrome/common/chrome_featur... File chrome/common/chrome_features.cc (right): https://codereview.chromium.org/2029263002/diff/1/chrome/common/chrome_featur... chrome/common/chrome_features.cc:61: // Enables or disables the Material Design version of chrome://settings. maybe add something about this affecting chrome://help as well?
https://codereview.chromium.org/2029263002/diff/1/chrome/browser/browser_abou... File chrome/browser/browser_about_handler.cc (right): https://codereview.chromium.org/2029263002/diff/1/chrome/browser/browser_abou... chrome/browser/browser_about_handler.cc:98: } else { can't this just be: } else if (!base::FeatureList::IsEnabled( features::kMaterialDesignSettingsFeature)) { host = chrome::kChromeUIUberHost; path = chrome::kChromeUISettingsHost + url->path(); } https://codereview.chromium.org/2029263002/diff/1/chrome/browser/browser_abou... chrome/browser/browser_about_handler.cc:109: features::kMaterialDesignSettingsFeature)) { same
https://codereview.chromium.org/2029263002/diff/1/chrome/browser/ui/webui/chr... File chrome/browser/ui/webui/chrome_web_ui_controller_factory.cc (right): https://codereview.chromium.org/2029263002/diff/1/chrome/browser/ui/webui/chr... chrome/browser/ui/webui/chrome_web_ui_controller_factory.cc:416: return &NewWebUI<settings::MdSettingsUI>; i think it'd make sense to do one of two things here: 1) group by host 2) group by webui i.e. if (url is md-settings) return md settings if (url is settings-frame) return settings if (url is settings) return md if md is on, else settings OR if (md-settings or settings URL and md is on) return md if (settings-frame or settings URL and md is off) return settings
On 2016/06/01 20:52:57, Dan Beam wrote: > fun fact: you also need to shoehorn in the new MD user manager on this flag as > well! luck you! lucky* you https://codereview.chromium.org/2029263002/diff/1/chrome/browser/ui/webui/set... File chrome/browser/ui/webui/settings/md_settings_ui.cc (right): https://codereview.chromium.org/2029263002/diff/1/chrome/browser/ui/webui/set... chrome/browser/ui/webui/settings/md_settings_ui.cc:89: content::WebUIDataSource::Create(html_host); On 2016/06/01 20:52:57, Dan Beam wrote: > instead of |html_host|, could this web web_ui->GetURL().host()? web_ui->GetWebContents()->GetVisibleUrl().host() **
I like "luck you" - I'll add it to my repertoire ;) https://codereview.chromium.org/2029263002/diff/1/chrome/browser/ui/webui/chr... File chrome/browser/ui/webui/chrome_web_ui_controller_factory.cc (right): https://codereview.chromium.org/2029263002/diff/1/chrome/browser/ui/webui/chr... chrome/browser/ui/webui/chrome_web_ui_controller_factory.cc:416: return &NewWebUI<settings::MdSettingsUI>; On 2016/06/01 20:59:49, Dan Beam wrote: > i think it'd make sense to do one of two things here: > > 1) group by host > 2) group by webui > > i.e. > > if (url is md-settings) > return md settings > > if (url is settings-frame) > return settings > > if (url is settings) > return md if md is on, else settings > > OR > > if (md-settings or settings URL and md is on) > return md > > if (settings-frame or settings URL and md is off) > return settings I'm following the established pattern - all material design switches are here :) https://codereview.chromium.org/2029263002/diff/1/chrome/browser/ui/webui/set... File chrome/browser/ui/webui/settings/md_settings_ui.cc (right): https://codereview.chromium.org/2029263002/diff/1/chrome/browser/ui/webui/set... chrome/browser/ui/webui/settings/md_settings_ui.cc:89: content::WebUIDataSource::Create(html_host); On 2016/06/01 21:01:09, Dan Beam wrote: > On 2016/06/01 20:52:57, Dan Beam wrote: > > instead of |html_host|, could this web web_ui->GetURL().host()? > > web_ui->GetWebContents()->GetVisibleUrl().host() ** Hm. I wonder if that could exploit routing problems (since now *any* URL that manages to reach the handler will give whatever URL is currently visible access.)
https://codereview.chromium.org/2029263002/diff/1/chrome/browser/ui/webui/chr... File chrome/browser/ui/webui/chrome_web_ui_controller_factory.cc (right): https://codereview.chromium.org/2029263002/diff/1/chrome/browser/ui/webui/chr... chrome/browser/ui/webui/chrome_web_ui_controller_factory.cc:416: return &NewWebUI<settings::MdSettingsUI>; On 2016/06/01 23:29:35, groby wrote: > On 2016/06/01 20:59:49, Dan Beam wrote: > > i think it'd make sense to do one of two things here: > > > > 1) group by host > > 2) group by webui > > > > i.e. > > > > if (url is md-settings) > > return md settings > > > > if (url is settings-frame) > > return settings > > > > if (url is settings) > > return md if md is on, else settings > > > > OR > > > > if (md-settings or settings URL and md is on) > > return md > > > > if (settings-frame or settings URL and md is off) > > return settings > > I'm following the established pattern - all material design switches are here :) ah, i see: UberUI creates it's own WebUIs for the subpages because it's a special flower. aight, this makes sense then
what does "chrome://md-settings will serve new settings, unless the flag is off." mean? shouldn't md-settings always work?
On 2016/06/02 19:31:20, Dan Beam wrote: > what does "chrome://md-settings will serve new settings, unless the flag is > off." mean? shouldn't md-settings always work? That's the question I asked in the first message :) If we _do_ keep it around, we'll need to dynamically retrieve the URL for the MdSettingsUi via GetVisibleUrl. I *think* that's valid when we create MdSettingsUi - but I don't know the exact lifetime details. Can you confirm?
On 2016/06/01 20:52:57, Dan Beam wrote: > fun fact: you also need to shoehorn in the new MD user manager on this flag as > well! luck you! What specifically do you mean by "shoehorn"? Force it on when md-settings are on? Change URL mappings?
On 2016/06/02 19:43:44, groby wrote: > On 2016/06/02 19:31:20, Dan Beam wrote: > > what does "chrome://md-settings will serve new settings, unless the flag is > > off." mean? shouldn't md-settings always work? > > That's the question I asked in the first message :) > > If we _do_ keep it around, we'll need to dynamically retrieve the URL for the > MdSettingsUi via GetVisibleUrl. I *think* that's valid when we create > MdSettingsUi - but I don't know the exact lifetime details. Can you confirm? yes, keep md-settings permanently on for now
On 2016/06/02 20:00:56, groby wrote: > On 2016/06/01 20:52:57, Dan Beam wrote: > > fun fact: you also need to shoehorn in the new MD user manager on this flag as > > well! luck you! > > What specifically do you mean by "shoehorn"? Force it on when md-settings are > on? Change URL mappings? the new settings delegates the ability to create users to the user manager (I think), so just make sure adding/removing users still works
Fixes are in. Pretty much works, except for one oddity: If your homepage settings are "where you left off", chrome://settings fails to load _at startup_ if md settings is disabled. A reload fixes that issue. Puzzled, looking, suggestions appreciated. https://codereview.chromium.org/2029263002/diff/1/chrome/browser/browser_abou... File chrome/browser/browser_about_handler.cc (right): https://codereview.chromium.org/2029263002/diff/1/chrome/browser/browser_abou... chrome/browser/browser_about_handler.cc:98: } else { On 2016/06/01 20:54:50, Dan Beam wrote: > can't this just be: > > } else if (!base::FeatureList::IsEnabled( > features::kMaterialDesignSettingsFeature)) { > host = chrome::kChromeUIUberHost; > path = chrome::kChromeUISettingsHost + url->path(); > } I kind of like explicitly calling out the alternatives - so I called it out in the comments :) (Done) https://codereview.chromium.org/2029263002/diff/1/chrome/browser/browser_abou... chrome/browser/browser_about_handler.cc:109: features::kMaterialDesignSettingsFeature)) { On 2016/06/01 20:54:50, Dan Beam wrote: > same Done. https://codereview.chromium.org/2029263002/diff/1/chrome/browser/ui/webui/set... File chrome/browser/ui/webui/settings/md_settings_ui.cc (right): https://codereview.chromium.org/2029263002/diff/1/chrome/browser/ui/webui/set... chrome/browser/ui/webui/settings/md_settings_ui.cc:89: content::WebUIDataSource::Create(html_host); On 2016/06/01 21:01:09, Dan Beam wrote: > On 2016/06/01 20:52:57, Dan Beam wrote: > > instead of |html_host|, could this web web_ui->GetURL().host()? > > web_ui->GetWebContents()->GetVisibleUrl().host() ** Tentatively done. Still feeling queasy about it. https://codereview.chromium.org/2029263002/diff/1/chrome/common/chrome_featur... File chrome/common/chrome_features.cc (right): https://codereview.chromium.org/2029263002/diff/1/chrome/common/chrome_featur... chrome/common/chrome_features.cc:61: // Enables or disables the Material Design version of chrome://settings. On 2016/06/01 20:52:57, Dan Beam wrote: > maybe add something about this affecting chrome://help as well? Done.
otherwise this seems fine to me, but where does the user manager come in? https://codereview.chromium.org/2029263002/diff/20001/chrome/browser/browser_... File chrome/browser/browser_about_handler.cc (right): https://codereview.chromium.org/2029263002/diff/20001/chrome/browser/browser_... chrome/browser/browser_about_handler.cc:108: } else { this } else { seems wrong https://codereview.chromium.org/2029263002/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/settings/md_settings_ui.cc (right): https://codereview.chromium.org/2029263002/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/md_settings_ui.cc:86: // either chrome://settings or chrome://md-settings. if you're worried about it, const GURL url = web_ui->GetWebContents()->GetVisibleURL(); DCHECK(url.GetOrigin() == GURL(chrome::kChromeUISettingsHost).GetOrigin() || url.GetOrigin() == GURL(chrome::kChromeUIMdSettingsHost).GetOrigin()); content::WebUIDataSource* html_source = content::WebUIDataSource::Create(url.host()); https://codereview.chromium.org/2029263002/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/uber/uber_ui_browsertest.cc (right): https://codereview.chromium.org/2029263002/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/uber/uber_ui_browsertest.cc:8: #include "base/command_line.h" #include "base/feature_list.h"
groby@chromium.org changed reviewers: + thakis@chromium.org
PTAL - dbeam: webui nico: not webui https://codereview.chromium.org/2029263002/diff/20001/chrome/browser/browser_... File chrome/browser/browser_about_handler.cc (right): https://codereview.chromium.org/2029263002/diff/20001/chrome/browser/browser_... chrome/browser/browser_about_handler.cc:108: } else { On 2016/06/03 03:01:43, Dan Beam wrote: > this } else { seems wrong Sure does. Thanks for catching. https://codereview.chromium.org/2029263002/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/settings/md_settings_ui.cc (right): https://codereview.chromium.org/2029263002/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/md_settings_ui.cc:86: // either chrome://settings or chrome://md-settings. On 2016/06/03 03:01:43, Dan Beam wrote: > if you're worried about it, > > const GURL url = web_ui->GetWebContents()->GetVisibleURL(); > DCHECK(url.GetOrigin() == GURL(chrome::kChromeUISettingsHost).GetOrigin() || > url.GetOrigin() == GURL(chrome::kChromeUIMdSettingsHost).GetOrigin()); > content::WebUIDataSource* html_source = > content::WebUIDataSource::Create(url.host()); Done. And given that this is a possible privilege escalation, it's a CHECK. https://codereview.chromium.org/2029263002/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/uber/uber_ui_browsertest.cc (right): https://codereview.chromium.org/2029263002/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/uber/uber_ui_browsertest.cc:8: #include "base/command_line.h" On 2016/06/03 03:01:43, Dan Beam wrote: > #include "base/feature_list.h" Done.
looks really good to me, just one last question in chrome_web_ui_controller_factory.cc https://codereview.chromium.org/2029263002/diff/60001/chrome/browser/profiles... File chrome/browser/profiles/profile_window.cc (right): https://codereview.chromium.org/2029263002/diff/60001/chrome/browser/profiles... chrome/browser/profiles/profile_window.cc:137: bool useMaterialDesignUserManager = use_material_design_user_manager this is not cocoa or js ;) also, super optional nit: i think use_md_user_manager would probably be equally explanatory and more succinct https://codereview.chromium.org/2029263002/diff/60001/chrome/browser/ui/webui... File chrome/browser/ui/webui/chrome_web_ui_controller_factory.cc (right): https://codereview.chromium.org/2029263002/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/chrome_web_ui_controller_factory.cc:513: if (url.host() == chrome::kChromeUIUserManagerHost) should this also be checking whether MD settings is enabled?
https://codereview.chromium.org/2029263002/diff/60001/chrome/browser/ui/webui... File chrome/browser/ui/webui/chrome_web_ui_controller_factory.cc (right): https://codereview.chromium.org/2029263002/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/chrome_web_ui_controller_factory.cc:513: if (url.host() == chrome::kChromeUIUserManagerHost) On 2016/06/07 00:25:08, Dan Beam wrote: > should this also be checking whether MD settings is enabled? No, because we'd never get that host in the first place - the other check switches us to a different host.
https://codereview.chromium.org/2029263002/diff/60001/chrome/browser/ui/webui... File chrome/browser/ui/webui/chrome_web_ui_controller_factory.cc (right): https://codereview.chromium.org/2029263002/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/chrome_web_ui_controller_factory.cc:513: if (url.host() == chrome::kChromeUIUserManagerHost) On 2016/06/07 00:27:39, groby wrote: > On 2016/06/07 00:25:08, Dan Beam wrote: > > should this also be checking whether MD settings is enabled? > > No, because we'd never get that host in the first place - the other check > switches us to a different host. yeah, maybe this is where the check should go instead of in the other place why would we ever want the same Chrome session to show different versions of the user manager?
https://codereview.chromium.org/2029263002/diff/60001/chrome/browser/ui/webui... File chrome/browser/ui/webui/chrome_web_ui_controller_factory.cc (right): https://codereview.chromium.org/2029263002/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/chrome_web_ui_controller_factory.cc:513: if (url.host() == chrome::kChromeUIUserManagerHost) On 2016/06/07 00:32:36, Dan Beam wrote: > On 2016/06/07 00:27:39, groby wrote: > > On 2016/06/07 00:25:08, Dan Beam wrote: > > > should this also be checking whether MD settings is enabled? > > > > No, because we'd never get that host in the first place - the other check > > switches us to a different host. > > yeah, maybe this is where the check should go instead of in the other place > > why would we ever want the same Chrome session to show different versions of the > user manager? You'd have to ask the user manager team :) I'd like to constrain this CL to making settings work, without changing the logic for other features.
dbeam@chromium.org changed reviewers: + mahmadi@chromium.org
https://codereview.chromium.org/2029263002/diff/60001/chrome/browser/ui/webui... File chrome/browser/ui/webui/chrome_web_ui_controller_factory.cc (right): https://codereview.chromium.org/2029263002/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/chrome_web_ui_controller_factory.cc:513: if (url.host() == chrome::kChromeUIUserManagerHost) On 2016/06/08 00:22:41, groby wrote: > On 2016/06/07 00:32:36, Dan Beam wrote: > > On 2016/06/07 00:27:39, groby wrote: > > > On 2016/06/07 00:25:08, Dan Beam wrote: > > > > should this also be checking whether MD settings is enabled? > > > > > > No, because we'd never get that host in the first place - the other check > > > switches us to a different host. > > > > yeah, maybe this is where the check should go instead of in the other place > > > > why would we ever want the same Chrome session to show different versions of > the > > user manager? > > You'd have to ask the user manager team :) +mahmadi > > I'd like to constrain this CL to making settings work, without changing the > logic for other features. this CL might the MD user manager not work correctly
On 2016/06/08 00:24:25, Dan Beam wrote: > https://codereview.chromium.org/2029263002/diff/60001/chrome/browser/ui/webui... > File chrome/browser/ui/webui/chrome_web_ui_controller_factory.cc (right): > > https://codereview.chromium.org/2029263002/diff/60001/chrome/browser/ui/webui... > chrome/browser/ui/webui/chrome_web_ui_controller_factory.cc:513: if (url.host() > == chrome::kChromeUIUserManagerHost) > On 2016/06/08 00:22:41, groby wrote: > > On 2016/06/07 00:32:36, Dan Beam wrote: > > > On 2016/06/07 00:27:39, groby wrote: > > > > On 2016/06/07 00:25:08, Dan Beam wrote: > > > > > should this also be checking whether MD settings is enabled? > > > > > > > > No, because we'd never get that host in the first place - the other check > > > > switches us to a different host. > > > > > > yeah, maybe this is where the check should go instead of in the other place > > > > > > why would we ever want the same Chrome session to show different versions of > > the > > > user manager? > > > > You'd have to ask the user manager team :) > > +mahmadi > > > > > I'd like to constrain this CL to making settings work, without changing the > > logic for other features. > > this CL might the MD user manager not work correctly In what regard? We're forcing md-user-manager on if md-settings is on, as agreed on e-mail thread. (profile_window.cc is the only place that checks that flag). chrome://user-manager and chrome://md-user-manager were simultaneously available before this change, and will be after this change.
On 2016/06/08 00:28:16, groby wrote: > On 2016/06/08 00:24:25, Dan Beam wrote: > > > https://codereview.chromium.org/2029263002/diff/60001/chrome/browser/ui/webui... > > File chrome/browser/ui/webui/chrome_web_ui_controller_factory.cc (right): > > > > > https://codereview.chromium.org/2029263002/diff/60001/chrome/browser/ui/webui... > > chrome/browser/ui/webui/chrome_web_ui_controller_factory.cc:513: if > (url.host() > > == chrome::kChromeUIUserManagerHost) > > On 2016/06/08 00:22:41, groby wrote: > > > On 2016/06/07 00:32:36, Dan Beam wrote: > > > > On 2016/06/07 00:27:39, groby wrote: > > > > > On 2016/06/07 00:25:08, Dan Beam wrote: > > > > > > should this also be checking whether MD settings is enabled? > > > > > > > > > > No, because we'd never get that host in the first place - the other > check > > > > > switches us to a different host. > > > > > > > > yeah, maybe this is where the check should go instead of in the other > place > > > > > > > > why would we ever want the same Chrome session to show different versions > of > > > the > > > > user manager? > > > > > > You'd have to ask the user manager team :) > > > > +mahmadi > > > > > > > > I'd like to constrain this CL to making settings work, without changing the > > > logic for other features. > > > > this CL might the MD user manager not work correctly > > In what regard? We're forcing md-user-manager on if md-settings is on, as agreed > on e-mail thread. (profile_window.cc is the only place that checks that flag). > > chrome://user-manager and chrome://md-user-manager were simultaneously available > before this change, and will be after this change. if the URL is hidden and we don't have to modify the browser tests, lgtm I guess
On 2016/06/08 00:33:25, Dan Beam wrote: > On 2016/06/08 00:28:16, groby wrote: > > On 2016/06/08 00:24:25, Dan Beam wrote: > > > > > > https://codereview.chromium.org/2029263002/diff/60001/chrome/browser/ui/webui... > > > File chrome/browser/ui/webui/chrome_web_ui_controller_factory.cc (right): > > > > > > > > > https://codereview.chromium.org/2029263002/diff/60001/chrome/browser/ui/webui... > > > chrome/browser/ui/webui/chrome_web_ui_controller_factory.cc:513: if > > (url.host() > > > == chrome::kChromeUIUserManagerHost) > > > On 2016/06/08 00:22:41, groby wrote: > > > > On 2016/06/07 00:32:36, Dan Beam wrote: > > > > > On 2016/06/07 00:27:39, groby wrote: > > > > > > On 2016/06/07 00:25:08, Dan Beam wrote: > > > > > > > should this also be checking whether MD settings is enabled? > > > > > > > > > > > > No, because we'd never get that host in the first place - the other > > check > > > > > > switches us to a different host. > > > > > > > > > > yeah, maybe this is where the check should go instead of in the other > > place > > > > > > > > > > why would we ever want the same Chrome session to show different > versions > > of > > > > the > > > > > user manager? > > > > > > > > You'd have to ask the user manager team :) > > > > > > +mahmadi > > > > > > > > > > > I'd like to constrain this CL to making settings work, without changing > the > > > > logic for other features. > > > > > > this CL might the MD user manager not work correctly > > > > In what regard? We're forcing md-user-manager on if md-settings is on, as > agreed > > on e-mail thread. (profile_window.cc is the only place that checks that flag). > > > > chrome://user-manager and chrome://md-user-manager were simultaneously > available > > before this change, and will be after this change. > > if the URL is hidden and we don't have to modify the browser tests, lgtm I guess lgtm
On 2016/06/08 12:38:54, Moe wrote: > On 2016/06/08 00:33:25, Dan Beam wrote: > > On 2016/06/08 00:28:16, groby wrote: > > > On 2016/06/08 00:24:25, Dan Beam wrote: > > > > > > > > > > https://codereview.chromium.org/2029263002/diff/60001/chrome/browser/ui/webui... > > > > File chrome/browser/ui/webui/chrome_web_ui_controller_factory.cc (right): > > > > > > > > > > > > > > https://codereview.chromium.org/2029263002/diff/60001/chrome/browser/ui/webui... > > > > chrome/browser/ui/webui/chrome_web_ui_controller_factory.cc:513: if > > > (url.host() > > > > == chrome::kChromeUIUserManagerHost) > > > > On 2016/06/08 00:22:41, groby wrote: > > > > > On 2016/06/07 00:32:36, Dan Beam wrote: > > > > > > On 2016/06/07 00:27:39, groby wrote: > > > > > > > On 2016/06/07 00:25:08, Dan Beam wrote: > > > > > > > > should this also be checking whether MD settings is enabled? > > > > > > > > > > > > > > No, because we'd never get that host in the first place - the other > > > check > > > > > > > switches us to a different host. > > > > > > > > > > > > yeah, maybe this is where the check should go instead of in the other > > > place > > > > > > > > > > > > why would we ever want the same Chrome session to show different > > versions > > > of > > > > > the > > > > > > user manager? > > > > > > > > > > You'd have to ask the user manager team :) > > > > > > > > +mahmadi > > > > > > > > > > > > > > I'd like to constrain this CL to making settings work, without changing > > the > > > > > logic for other features. > > > > > > > > this CL might the MD user manager not work correctly > > > > > > In what regard? We're forcing md-user-manager on if md-settings is on, as > > agreed > > > on e-mail thread. (profile_window.cc is the only place that checks that > flag). > > > > > > chrome://user-manager and chrome://md-user-manager were simultaneously > > available > > > before this change, and will be after this change. > > > > if the URL is hidden and we don't have to modify the browser tests, lgtm I > guess > > lgtm Moe: Did you review the entire non-WebUI part, or just user-manager related? (If the former, I don't have to nag Nico :)
On 2016/06/08 16:47:01, groby wrote: > On 2016/06/08 12:38:54, Moe wrote: > > On 2016/06/08 00:33:25, Dan Beam wrote: > > > On 2016/06/08 00:28:16, groby wrote: > > > > On 2016/06/08 00:24:25, Dan Beam wrote: > > > > > > > > > > > > > > > https://codereview.chromium.org/2029263002/diff/60001/chrome/browser/ui/webui... > > > > > File chrome/browser/ui/webui/chrome_web_ui_controller_factory.cc > (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2029263002/diff/60001/chrome/browser/ui/webui... > > > > > chrome/browser/ui/webui/chrome_web_ui_controller_factory.cc:513: if > > > > (url.host() > > > > > == chrome::kChromeUIUserManagerHost) > > > > > On 2016/06/08 00:22:41, groby wrote: > > > > > > On 2016/06/07 00:32:36, Dan Beam wrote: > > > > > > > On 2016/06/07 00:27:39, groby wrote: > > > > > > > > On 2016/06/07 00:25:08, Dan Beam wrote: > > > > > > > > > should this also be checking whether MD settings is enabled? > > > > > > > > > > > > > > > > No, because we'd never get that host in the first place - the > other > > > > check > > > > > > > > switches us to a different host. > > > > > > > > > > > > > > yeah, maybe this is where the check should go instead of in the > other > > > > place > > > > > > > > > > > > > > why would we ever want the same Chrome session to show different > > > versions > > > > of > > > > > > the > > > > > > > user manager? > > > > > > > > > > > > You'd have to ask the user manager team :) > > > > > > > > > > +mahmadi > > > > > > > > > > > > > > > > > I'd like to constrain this CL to making settings work, without > changing > > > the > > > > > > logic for other features. > > > > > > > > > > this CL might the MD user manager not work correctly > > > > > > > > In what regard? We're forcing md-user-manager on if md-settings is on, as > > > agreed > > > > on e-mail thread. (profile_window.cc is the only place that checks that > > flag). > > > > > > > > chrome://user-manager and chrome://md-user-manager were simultaneously > > > available > > > > before this change, and will be after this change. > > > > > > if the URL is hidden and we don't have to modify the browser tests, lgtm I > > guess > > > > lgtm > > Moe: Did you review the entire non-WebUI part, or just user-manager related? (If > the former, I don't have to nag Nico :) I only looked at the user-manager related stuff. plus, I'm not an owner anyway :)
Nico: Friendly ping for non-webui code? http://i.imgur.com/4xPF0P9.gif https://codereview.chromium.org/2029263002/diff/60001/chrome/browser/profiles... File chrome/browser/profiles/profile_window.cc (right): https://codereview.chromium.org/2029263002/diff/60001/chrome/browser/profiles... chrome/browser/profiles/profile_window.cc:137: bool useMaterialDesignUserManager = On 2016/06/07 00:25:08, Dan Beam wrote: > use_material_design_user_manager > > this is not cocoa or js ;) > > also, super optional nit: i think use_md_user_manager would probably be equally > explanatory and more succinct Done.
ping thakis@
lgtm https://codereview.chromium.org/2029263002/diff/80001/chrome/browser/browser_... File chrome/browser/browser_about_handler.cc (right): https://codereview.chromium.org/2029263002/diff/80001/chrome/browser/browser_... chrome/browser/browser_about_handler.cc:93: if (::switches::AboutInSettingsEnabled()) { this looks a bit funny -- if i enable md settings but also about-in-settings, i don't get md settings?
https://codereview.chromium.org/2029263002/diff/80001/chrome/browser/browser_... File chrome/browser/browser_about_handler.cc (right): https://codereview.chromium.org/2029263002/diff/80001/chrome/browser/browser_... chrome/browser/browser_about_handler.cc:93: if (::switches::AboutInSettingsEnabled()) { On 2016/06/10 18:41:27, Nico (traveling...slow) wrote: > this looks a bit funny -- if i enable md settings but also about-in-settings, i > don't get md settings? The two flags are mutually exclusive, so any combination is "undefined behavior" ;) (about-in-settings happens in the old settings page, md-settings is an entirely new settings page)
The CQ bit was checked by groby@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dbeam@chromium.org, mahmadi@chromium.org Link to the patchset: https://codereview.chromium.org/2029263002/#ps80001 (title: "One last fix...")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2029263002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios-device on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-gn on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...) ios-simulator on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-gn on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...) mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by groby@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from thakis@chromium.org, dbeam@chromium.org, mahmadi@chromium.org Link to the patchset: https://codereview.chromium.org/2029263002/#ps100001 (title: "Rebase to HEAD")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2029263002/100001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios-simulator on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-gn on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...) mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by groby@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from thakis@chromium.org, dbeam@chromium.org, mahmadi@chromium.org Link to the patchset: https://codereview.chromium.org/2029263002/#ps140001 (title: "Removing unneeded file.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2029263002/140001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
On 2016/06/13 21:09:29, commit-bot: I haz the power wrote: > Try jobs failed on following builders: > mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...) Reviewers FYI: Rebased to HEAD, fixed a CHECK statement that checked using the webui host name as opposed to the full URL. Still landing, since functionality unchanged.
The CQ bit was checked by groby@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from thakis@chromium.org, dbeam@chromium.org, mahmadi@chromium.org Link to the patchset: https://codereview.chromium.org/2029263002/#ps160001 (title: "Fix URL CHECK statement. (Use URL, not Host)")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2029263002/160001
Message was sent while issue was closed.
Description was changed from ========== [MD Settings] Add feature to enable md-settings by default. chrome://settings will serve either old settings or md-settings, depending on flag. chrome://settings-frame will always serve old settings. chrome://md-settings will serve new settings, unless the flag is off. BUG=614758 ========== to ========== [MD Settings] Add feature to enable md-settings by default. chrome://settings will serve either old settings or md-settings, depending on flag. chrome://settings-frame will always serve old settings. chrome://md-settings will serve new settings, unless the flag is off. BUG=614758 ==========
Message was sent while issue was closed.
Committed patchset #9 (id:160001)
Message was sent while issue was closed.
CQ bit was unchecked
Message was sent while issue was closed.
Description was changed from ========== [MD Settings] Add feature to enable md-settings by default. chrome://settings will serve either old settings or md-settings, depending on flag. chrome://settings-frame will always serve old settings. chrome://md-settings will serve new settings, unless the flag is off. BUG=614758 ========== to ========== [MD Settings] Add feature to enable md-settings by default. chrome://settings will serve either old settings or md-settings, depending on flag. chrome://settings-frame will always serve old settings. chrome://md-settings will serve new settings, unless the flag is off. BUG=614758 Committed: https://crrev.com/12483f26c404123844df38d8e8083c1d6dde1426 Cr-Commit-Position: refs/heads/master@{#399640} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/12483f26c404123844df38d8e8083c1d6dde1426 Cr-Commit-Position: refs/heads/master@{#399640} |
