Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
895163002:
Add a development flag for the new material design Settings UI. (Closed)
|
Created:
5 years, 10 months ago by Oren Blasberg Modified:
5 years, 10 months ago CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd a development flag for the new material design Settings UI.
A follow up CL will make use of this flag to enable the chrome://md-settings/ url for the new settings page.
BUG=454961
Committed: https://crrev.com/5ee85fa8a1aa9a17becd12a51b44750cbf02b096
Cr-Commit-Position: refs/heads/master@{#314611}
Patch Set 1 #
Total comments: 8
Patch Set 2 : Shorten material-design to md. #Patch Set 3 : Remove is_android check in generated_resources. #
Total comments: 4
Patch Set 4 : Address nits. #
Total comments: 5
Patch Set 5 : Update histograms.xml -- required for the about_flags test to pass. #
Messages
Total messages: 32 (9 generated)
orenb@chromium.org changed reviewers: + jhawkins@chromium.org, stevenjb@chromium.org
https://codereview.chromium.org/895163002/diff/1/chrome/app/generated_resourc... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/895163002/diff/1/chrome/app/generated_resourc... chrome/app/generated_resources.grd:5410: + <if expr="not is_android"> Why is this gated on not is_android? https://codereview.chromium.org/895163002/diff/1/chrome/browser/about_flags.cc File chrome/browser/about_flags.cc (right): https://codereview.chromium.org/895163002/diff/1/chrome/browser/about_flags.c... chrome/browser/about_flags.cc:1985: "enable-material-design-settings", Let's shorten this to enable-md-settings. https://codereview.chromium.org/895163002/diff/1/chrome/common/chrome_switche... File chrome/common/chrome_switches.cc (right): https://codereview.chromium.org/895163002/diff/1/chrome/common/chrome_switche... chrome/common/chrome_switches.cc:455: const char kEnableMaterialDesignSettings[] = "enable-material-design-settings"; enable-md-settings
https://codereview.chromium.org/895163002/diff/1/chrome/app/generated_resourc... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/895163002/diff/1/chrome/app/generated_resourc... chrome/app/generated_resources.grd:5410: + <if expr="not is_android"> On 2015/02/03 22:08:46, James Hawkins wrote: > Why is this gated on not is_android? On Android, the settings UI isn't WebUI based so I figured it didn't make sense to expose the flag. Is this going to change? If so I can remove the gate. https://codereview.chromium.org/895163002/diff/1/chrome/browser/about_flags.cc File chrome/browser/about_flags.cc (right): https://codereview.chromium.org/895163002/diff/1/chrome/browser/about_flags.c... chrome/browser/about_flags.cc:1985: "enable-material-design-settings", On 2015/02/03 22:08:46, James Hawkins wrote: > Let's shorten this to enable-md-settings. Done. https://codereview.chromium.org/895163002/diff/1/chrome/common/chrome_switche... File chrome/common/chrome_switches.cc (right): https://codereview.chromium.org/895163002/diff/1/chrome/common/chrome_switche... chrome/common/chrome_switches.cc:455: const char kEnableMaterialDesignSettings[] = "enable-material-design-settings"; On 2015/02/03 22:08:46, James Hawkins wrote: > enable-md-settings Done.
https://codereview.chromium.org/895163002/diff/1/chrome/app/generated_resourc... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/895163002/diff/1/chrome/app/generated_resourc... chrome/app/generated_resources.grd:5410: + <if expr="not is_android"> On 2015/02/03 22:13:34, Oren Blasberg wrote: > On 2015/02/03 22:08:46, James Hawkins wrote: > > Why is this gated on not is_android? > > On Android, the settings UI isn't WebUI based so I figured it didn't make sense > to expose the flag. Is this going to change? If so I can remove the gate. I don't see why we need to complicate this logic just because Android exists. Let's just take that part off.
https://codereview.chromium.org/895163002/diff/1/chrome/app/generated_resourc... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/895163002/diff/1/chrome/app/generated_resourc... chrome/app/generated_resources.grd:5410: + <if expr="not is_android"> On 2015/02/03 22:15:22, James Hawkins wrote: > On 2015/02/03 22:13:34, Oren Blasberg wrote: > > On 2015/02/03 22:08:46, James Hawkins wrote: > > > Why is this gated on not is_android? > > > > On Android, the settings UI isn't WebUI based so I figured it didn't make > sense > > to expose the flag. Is this going to change? If so I can remove the gate. > > I don't see why we need to complicate this logic just because Android exists. > Let's just take that part off. Done.
LGTM with nits. https://codereview.chromium.org/895163002/diff/40001/chrome/app/generated_res... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/895163002/diff/40001/chrome/app/generated_res... chrome/app/generated_resources.grd:5414: + If enabled, the chrome://md-settings/ URL will load the Material Design settings page. nit: s/will load/loads/ https://codereview.chromium.org/895163002/diff/40001/chrome/common/chrome_swi... File chrome/common/chrome_switches.cc (right): https://codereview.chromium.org/895163002/diff/40001/chrome/common/chrome_swi... chrome/common/chrome_switches.cc:454: // Enables the material design Settings page/app. nit: s/page\/app/feature/.
Let's create a crbug.com issue for "Create material design settings page" and specify that for bug=. Otherwise LGTM
New patchsets have been uploaded after l-g-t-m from jhawkins@chromium.org,stevenjb@chromium.org
https://codereview.chromium.org/895163002/diff/40001/chrome/app/generated_res... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/895163002/diff/40001/chrome/app/generated_res... chrome/app/generated_resources.grd:5414: + If enabled, the chrome://md-settings/ URL will load the Material Design settings page. On 2015/02/03 22:23:46, James Hawkins wrote: > nit: s/will load/loads/ Done. https://codereview.chromium.org/895163002/diff/40001/chrome/common/chrome_swi... File chrome/common/chrome_switches.cc (right): https://codereview.chromium.org/895163002/diff/40001/chrome/common/chrome_swi... chrome/common/chrome_switches.cc:454: // Enables the material design Settings page/app. On 2015/02/03 22:23:46, James Hawkins wrote: > nit: s/page\/app/feature/. Done.
The CQ bit was checked by orenb@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/895163002/60001
dbeam@chromium.org changed reviewers: + dbeam@chromium.org
https://codereview.chromium.org/895163002/diff/60001/chrome/common/chrome_swi... File chrome/common/chrome_switches.h (right): https://codereview.chromium.org/895163002/diff/60001/chrome/common/chrome_swi... chrome/common/chrome_switches.h:134: extern const char kEnableLinkableEphemeralApps[]; why not just an --enable-material-design?
https://codereview.chromium.org/895163002/diff/60001/chrome/common/chrome_swi... File chrome/common/chrome_switches.h (right): https://codereview.chromium.org/895163002/diff/60001/chrome/common/chrome_swi... chrome/common/chrome_switches.h:134: extern const char kEnableLinkableEphemeralApps[]; On 2015/02/03 22:38:02, Dan Beam wrote: > why not just an --enable-material-design? I think the wide scope of that will not provide enough granularity as we expand the effort to other WebUI features (and even the Chrome of Chrome). --enable-md-featurename seems like the right granularity so one could test, say, md-downloads but have all other WebUI pages be the old codebase.
https://codereview.chromium.org/895163002/diff/60001/chrome/common/chrome_swi... File chrome/common/chrome_switches.h (right): https://codereview.chromium.org/895163002/diff/60001/chrome/common/chrome_swi... chrome/common/chrome_switches.h:134: extern const char kEnableLinkableEphemeralApps[]; On 2015/02/03 22:47:39, James Hawkins wrote: > On 2015/02/03 22:38:02, Dan Beam wrote: > > why not just an --enable-material-design? > > I think the wide scope of that will not provide enough granularity as we expand > the effort to other WebUI features (and even the Chrome of Chrome). > --enable-md-featurename seems like the right granularity so one could test, say, > md-downloads but have all other WebUI pages be the old codebase. ok, but will settings still be in an uber page?
https://codereview.chromium.org/895163002/diff/60001/chrome/common/chrome_swi... File chrome/common/chrome_switches.h (right): https://codereview.chromium.org/895163002/diff/60001/chrome/common/chrome_swi... chrome/common/chrome_switches.h:134: extern const char kEnableLinkableEphemeralApps[]; On 2015/02/03 22:48:33, Dan Beam wrote: > On 2015/02/03 22:47:39, James Hawkins wrote: > > On 2015/02/03 22:38:02, Dan Beam wrote: > > > why not just an --enable-material-design? > > > > I think the wide scope of that will not provide enough granularity as we > expand > > the effort to other WebUI features (and even the Chrome of Chrome). > > --enable-md-featurename seems like the right granularity so one could test, > say, > > md-downloads but have all other WebUI pages be the old codebase. > > ok, but will settings still be in an uber page? by this I mean: are we just swapping out just the settings tab (e.g. settings-frame://) or will settings/history/extensions/downloads/bookmarks/etc. be different tabs from now on? right now settings kind of needs to know it's embedded within the uber frame.
On 2015/02/03 22:54:50, Dan Beam wrote: > https://codereview.chromium.org/895163002/diff/60001/chrome/common/chrome_swi... > File chrome/common/chrome_switches.h (right): > > https://codereview.chromium.org/895163002/diff/60001/chrome/common/chrome_swi... > chrome/common/chrome_switches.h:134: extern const char > kEnableLinkableEphemeralApps[]; > On 2015/02/03 22:48:33, Dan Beam wrote: > > On 2015/02/03 22:47:39, James Hawkins wrote: > > > On 2015/02/03 22:38:02, Dan Beam wrote: > > > > why not just an --enable-material-design? > > > > > > I think the wide scope of that will not provide enough granularity as we > > expand > > > the effort to other WebUI features (and even the Chrome of Chrome). > > > --enable-md-featurename seems like the right granularity so one could test, > > say, > > > md-downloads but have all other WebUI pages be the old codebase. > > > > ok, but will settings still be in an uber page? > > by this I mean: are we just swapping out just the settings tab (e.g. > settings-frame://) or will settings/history/extensions/downloads/bookmarks/etc. > be different tabs from now on? > > right now settings kind of needs to know it's embedded within the uber frame. md-settings will not be in the Uber page. The existing settings impl will stay in the Uber page until we launch the md-settings feature (and probably still exist behind a --disable-md-settings flag for a while).
On 2015/02/03 22:59:54, James Hawkins wrote: > On 2015/02/03 22:54:50, Dan Beam wrote: > > > https://codereview.chromium.org/895163002/diff/60001/chrome/common/chrome_swi... > > File chrome/common/chrome_switches.h (right): > > > > > https://codereview.chromium.org/895163002/diff/60001/chrome/common/chrome_swi... > > chrome/common/chrome_switches.h:134: extern const char > > kEnableLinkableEphemeralApps[]; > > On 2015/02/03 22:48:33, Dan Beam wrote: > > > On 2015/02/03 22:47:39, James Hawkins wrote: > > > > On 2015/02/03 22:38:02, Dan Beam wrote: > > > > > why not just an --enable-material-design? > > > > > > > > I think the wide scope of that will not provide enough granularity as we > > > expand > > > > the effort to other WebUI features (and even the Chrome of Chrome). > > > > --enable-md-featurename seems like the right granularity so one could > test, > > > say, > > > > md-downloads but have all other WebUI pages be the old codebase. > > > > > > ok, but will settings still be in an uber page? > > > > by this I mean: are we just swapping out just the settings tab (e.g. > > settings-frame://) or will > settings/history/extensions/downloads/bookmarks/etc. > > be different tabs from now on? > > > > right now settings kind of needs to know it's embedded within the uber frame. > > md-settings will not be in the Uber page. The existing settings impl will stay > in the Uber page until we launch the md-settings feature (and probably still > exist behind a --disable-md-settings flag for a while). ah, i forgot what the mocks look like. sorry for the noise. lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
New patchsets have been uploaded after l-g-t-m from dbeam@chromium.org
orenb@chromium.org changed reviewers: + mpearson@chromium.org
mpearson@chromium.org: Please review changes in histograms.xml - thanks so much!
michaelpg@chromium.org changed reviewers: + michaelpg@chromium.org
lgtm I suggest adding the actual URL to the CL description. https://codereview.chromium.org/895163002/diff/60001/chrome/common/chrome_swi... File chrome/common/chrome_switches.h (right): https://codereview.chromium.org/895163002/diff/60001/chrome/common/chrome_swi... chrome/common/chrome_switches.h:134: extern const char kEnableLinkableEphemeralApps[]; On 2015/02/03 22:54:50, Dan Beam wrote: > On 2015/02/03 22:48:33, Dan Beam wrote: > > On 2015/02/03 22:47:39, James Hawkins wrote: > > > On 2015/02/03 22:38:02, Dan Beam wrote: > > > > why not just an --enable-material-design? > > > > > > I think the wide scope of that will not provide enough granularity as we > > expand > > > the effort to other WebUI features (and even the Chrome of Chrome). > > > --enable-md-featurename seems like the right granularity so one could test, > > say, > > > md-downloads but have all other WebUI pages be the old codebase. > > > > ok, but will settings still be in an uber page? > > by this I mean: are we just swapping out just the settings tab (e.g. > settings-frame://) or will settings/history/extensions/downloads/bookmarks/etc. > be different tabs from now on? > > right now settings kind of needs to know it's embedded within the uber frame. Why should we bother changing chrome://settings or chrome://settings-frame right now? It'll be a while before we have anything remotely functional let alone complete. The logic for where those pages go is complicated enough already, and differs on Chrome and Chrome OS unless the --enable-settings-window flag is changed. As we work on md-settings, it would be useful to be able to load the existing Settings as well (in another tab, not having to flip a flag and restart). We could make chrome://settings redirect and leave chrome://settings-frame as-is, but I don't see the point.
On 2015/02/03 23:01:43, Dan Beam wrote: > On 2015/02/03 22:59:54, James Hawkins wrote: > > On 2015/02/03 22:54:50, Dan Beam wrote: > > > > > > https://codereview.chromium.org/895163002/diff/60001/chrome/common/chrome_swi... > > > File chrome/common/chrome_switches.h (right): > > > > > > > > > https://codereview.chromium.org/895163002/diff/60001/chrome/common/chrome_swi... > > > chrome/common/chrome_switches.h:134: extern const char > > > kEnableLinkableEphemeralApps[]; > > > On 2015/02/03 22:48:33, Dan Beam wrote: > > > > On 2015/02/03 22:47:39, James Hawkins wrote: > > > > > On 2015/02/03 22:38:02, Dan Beam wrote: > > > > > > why not just an --enable-material-design? > > > > > > > > > > I think the wide scope of that will not provide enough granularity as we > > > > expand > > > > > the effort to other WebUI features (and even the Chrome of Chrome). > > > > > --enable-md-featurename seems like the right granularity so one could > > test, > > > > say, > > > > > md-downloads but have all other WebUI pages be the old codebase. > > > > > > > > ok, but will settings still be in an uber page? > > > > > > by this I mean: are we just swapping out just the settings tab (e.g. > > > settings-frame://) or will > > settings/history/extensions/downloads/bookmarks/etc. > > > be different tabs from now on? > > > > > > right now settings kind of needs to know it's embedded within the uber > frame. > > > > md-settings will not be in the Uber page. The existing settings impl will > stay > > in the Uber page until we launch the md-settings feature (and probably still > > exist behind a --disable-md-settings flag for a while). > > ah, i forgot what the mocks look like. sorry for the noise. lgtm Hmm didn't see this comment, ignore my response about chrome://settings then.
histograms.xml lgtm
The CQ bit was checked by orenb@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/895163002/80001
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/5ee85fa8a1aa9a17becd12a51b44750cbf02b096 Cr-Commit-Position: refs/heads/master@{#314611} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
