Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
313363004:
Separate Settings and Help from History and Extensions when settings-in- (Closed)
|
Created:
6 years, 6 months ago by michaelpg Modified:
6 years, 6 months ago CC:
chromium-reviews, arv+watch_chromium.org, stevenjb Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionSeparate Settings and Help from History and Extensions when settings-in-a-window is enabled.
With --enable-settings-window, only Settings and Help should be shown in the left nav bar when opening settings or help. Likewise, history and extensions should be the only two nav items when loading those pages.
This is a temporary change to unblock Settings in a Window. Going forward, we will remove the left nav entirely from the Settings window, so the logic for having nav item groups here is only temporary. See issue 294023013 for how this will be done after the branch.
BUG=375425
R=dbeam@chromium.org,sky@chromium.org
TBR=estade@chromium.org
NOTRY=true
# win_chromium_x64_rel flaky failures
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=276327
Patch Set 1 #
Total comments: 5
Patch Set 2 : feedback #
Total comments: 20
Patch Set 3 : JS edits #
Total comments: 6
Patch Set 4 : #Patch Set 5 : #Patch Set 6 : rebase (no conflicts) #
Messages
Total messages: 31 (0 generated)
Dan, the previous CL you were reviewing is being put on hold because it was decided that we couldn't launch settings in a window without keeping the link to the Help page. Instead of removing the left sidebar, we remove the history/extensions items from the sidebar in settings/help and vice versa.
https://codereview.chromium.org/313363004/diff/1/chrome/browser/ui/settings_w... File chrome/browser/ui/settings_window_manager.h (right): https://codereview.chromium.org/313363004/diff/1/chrome/browser/ui/settings_w... chrome/browser/ui/settings_window_manager.h:23: bool SettingsWindowEnabled(); I wouldn't expose this here, I would just create a local function in whatever .cc file needs it. https://codereview.chromium.org/313363004/diff/1/chrome/browser/ui/webui/uber... File chrome/browser/ui/webui/uber/uber_ui.cc (right): https://codereview.chromium.org/313363004/diff/1/chrome/browser/ui/webui/uber... chrome/browser/ui/webui/uber/uber_ui.cc:109: chrome::SettingsWindowEnabled() ? "group2" : "group1")); Maybe names these "settings_group" and "other_group"?
What do you need me to review here?
On 2014/06/05 23:11:12, sky wrote: > What do you need me to review here? I'll need a stamp from an owner of chrome/browser/ui. Specifically these files, which stevenjb will review: chrome/browser/ui/chrome_pages.cc chrome/browser/ui/settings_window_manager* Thanks.
https://codereview.chromium.org/313363004/diff/1/chrome/browser/ui/settings_w... File chrome/browser/ui/settings_window_manager.h (right): https://codereview.chromium.org/313363004/diff/1/chrome/browser/ui/settings_w... chrome/browser/ui/settings_window_manager.h:23: bool SettingsWindowEnabled(); On 2014/06/05 21:59:05, stevenjb wrote: > I wouldn't expose this here, I would just create a local function in whatever > .cc file needs it. And name it IsSettingsWindowEnabled.
https://codereview.chromium.org/313363004/diff/1/chrome/browser/ui/settings_w... File chrome/browser/ui/settings_window_manager.h (right): https://codereview.chromium.org/313363004/diff/1/chrome/browser/ui/settings_w... chrome/browser/ui/settings_window_manager.h:23: bool SettingsWindowEnabled(); On 2014/06/05 23:48:45, sky wrote: > On 2014/06/05 21:59:05, stevenjb wrote: > > I wouldn't expose this here, I would just create a local function in whatever > > .cc file needs it. > > And name it IsSettingsWindowEnabled. Done. (uber_ui.cc, chrome_pages.cc) https://codereview.chromium.org/313363004/diff/1/chrome/browser/ui/webui/uber... File chrome/browser/ui/webui/uber/uber_ui.cc (right): https://codereview.chromium.org/313363004/diff/1/chrome/browser/ui/webui/uber... chrome/browser/ui/webui/uber/uber_ui.cc:109: chrome::SettingsWindowEnabled() ? "group2" : "group1")); On 2014/06/05 21:59:05, stevenjb wrote: > Maybe names these "settings_group" and "other_group"? Done.
https://codereview.chromium.org/313363004/diff/40001/chrome/browser/resources... File chrome/browser/resources/uber/uber_frame.html (right): https://codereview.chromium.org/313363004/diff/40001/chrome/browser/resources... chrome/browser/resources/uber/uber_frame.html:20: group:historyGroup" hidden="hidden"> s/="hidden"//g https://codereview.chromium.org/313363004/diff/40001/chrome/browser/resources... File chrome/browser/resources/uber/uber_frame.js (right): https://codereview.chromium.org/313363004/diff/40001/chrome/browser/resources... chrome/browser/resources/uber/uber_frame.js:80: showNavItems(); shouldn't this only be invoked if IsSettingsWindowEnabled()? https://codereview.chromium.org/313363004/diff/40001/chrome/browser/resources... chrome/browser/resources/uber/uber_frame.js:91: if (lastSelectedNavItem) ^ it would seem that there might not be a selected nav item from this code https://codereview.chromium.org/313363004/diff/40001/chrome/browser/resources... chrome/browser/resources/uber/uber_frame.js:101: var selectedNavItem = document.querySelector('li.selected'); nit: make a function getSelectedNavItem() { return document.querySelector('li.selected'); } a re-use here and other places https://codereview.chromium.org/313363004/diff/40001/chrome/browser/resources... chrome/browser/resources/uber/uber_frame.js:101: var selectedNavItem = document.querySelector('li.selected'); what happens if this returns null? https://codereview.chromium.org/313363004/diff/40001/chrome/browser/resources... chrome/browser/resources/uber/uber_frame.js:105: selectedNavItem.getAttribute('group')); nit: needless () https://codereview.chromium.org/313363004/diff/40001/chrome/browser/resources... chrome/browser/resources/uber/uber_frame.js:105: selectedNavItem.getAttribute('group')); you don't ever actually use selectedNavItem itself, I'd just recommend var selectedNavItem = getSelectectedNavItem(); if (!selectedNavItem) // or assert(selectedNavItem) return; var selectedGroup = selectedNavItem.getAttribute('group'); // use |selectedGroup| https://codereview.chromium.org/313363004/diff/40001/chrome/browser/ui/settin... File chrome/browser/ui/settings_window_manager.h (right): https://codereview.chromium.org/313363004/diff/40001/chrome/browser/ui/settin... chrome/browser/ui/settings_window_manager.h:33: // Shows a chrome:// page (e.g. Settings, Help) in an an existing system ^ shouldn't this be About, now? ;) https://codereview.chromium.org/313363004/diff/40001/chrome/browser/ui/webui/... File chrome/browser/ui/webui/uber/uber_ui.cc (right): https://codereview.chromium.org/313363004/diff/40001/chrome/browser/ui/webui/... chrome/browser/ui/webui/uber/uber_ui.cc:38: bool IsSettingsWindowEnabled() { why another one?! https://codereview.chromium.org/313363004/diff/40001/chrome/browser/ui/webui/... chrome/browser/ui/webui/uber/uber_ui.cc:113: base::string16 settingsGroup(ASCIIToUTF16("settings_group")); cpp_vars_like_this
https://codereview.chromium.org/313363004/diff/40001/chrome/browser/ui/webui/... File chrome/browser/ui/webui/uber/uber_ui.cc (right): https://codereview.chromium.org/313363004/diff/40001/chrome/browser/ui/webui/... chrome/browser/ui/webui/uber/uber_ui.cc:38: bool IsSettingsWindowEnabled() { On 2014/06/06 21:21:23, Dan Beam wrote: > why another one?! you and stevenjb@ need to agree to disagree :-) https://codereview.chromium.org/313363004/diff/1/chrome/browser/ui/settings_w... > chrome/browser/ui/settings_window_manager.h:23: bool SettingsWindowEnabled(); > I wouldn't expose this here, I would just create a local function in whatever > .cc file needs it.
lgtm https://codereview.chromium.org/313363004/diff/40001/chrome/browser/ui/webui/... File chrome/browser/ui/webui/uber/uber_ui.cc (right): https://codereview.chromium.org/313363004/diff/40001/chrome/browser/ui/webui/... chrome/browser/ui/webui/uber/uber_ui.cc:38: bool IsSettingsWindowEnabled() { On 2014/06/06 21:34:58, Michael Giuffrida wrote: > On 2014/06/06 21:21:23, Dan Beam wrote: > > why another one?! > > you and stevenjb@ need to agree to disagree :-) > > https://codereview.chromium.org/313363004/diff/1/chrome/browser/ui/settings_w... > > chrome/browser/ui/settings_window_manager.h:23: bool SettingsWindowEnabled(); > > I wouldn't expose this here, I would just create a local function in whatever > > .cc file needs it. This doesn't do anything interesting, it's just shorthand, no need to expose it in a header. (It will go away with the flag anyway). If it were more than two places I'd add a helper to chrome_switches.cc.
https://codereview.chromium.org/313363004/diff/40001/chrome/browser/ui/webui/... File chrome/browser/ui/webui/uber/uber_ui.cc (right): https://codereview.chromium.org/313363004/diff/40001/chrome/browser/ui/webui/... chrome/browser/ui/webui/uber/uber_ui.cc:38: bool IsSettingsWindowEnabled() { On 2014/06/06 21:38:27, stevenjb wrote: > On 2014/06/06 21:34:58, Michael Giuffrida wrote: > > On 2014/06/06 21:21:23, Dan Beam wrote: > > > why another one?! > > > > you and stevenjb@ need to agree to disagree :-) > > > > > https://codereview.chromium.org/313363004/diff/1/chrome/browser/ui/settings_w... > > > chrome/browser/ui/settings_window_manager.h:23: bool > SettingsWindowEnabled(); > > > I wouldn't expose this here, I would just create a local function in > whatever > > > .cc file needs it. > This doesn't do anything interesting, it's just shorthand, no need to expose it > in a header. (It will go away with the flag anyway). If it were more than two > places I'd add a helper to chrome_switches.cc. it's fine
https://codereview.chromium.org/313363004/diff/40001/chrome/browser/resources... File chrome/browser/resources/uber/uber_frame.html (right): https://codereview.chromium.org/313363004/diff/40001/chrome/browser/resources... chrome/browser/resources/uber/uber_frame.html:20: group:historyGroup" hidden="hidden"> On 2014/06/06 21:21:23, Dan Beam wrote: > s/="hidden"//g Done. https://codereview.chromium.org/313363004/diff/40001/chrome/browser/resources... File chrome/browser/resources/uber/uber_frame.js (right): https://codereview.chromium.org/313363004/diff/40001/chrome/browser/resources... chrome/browser/resources/uber/uber_frame.js:80: showNavItems(); On 2014/06/06 21:21:23, Dan Beam wrote: > shouldn't this only be invoked if IsSettingsWindowEnabled()? I've made the nav items hidden to prevent the flicker you'd see in --enable-settings-window. Otherwise all nav items would show and then two would disappear. Because of that, showNavItems needs to be called at least once. changeSelection is called the first time the page loads to set the initial selection. The alternative is to 1. use .style.display instead of hidden attribute 2. set display attributes to "none" if --enable-settings-window 3. add "isSettingsWindow" to the data source for uber_frame.js 4. if !isSettingsWindow call showNavItems from changeSelection which in the case of !isSettingsWindow would be faster for some definition of speed. But this is simpler and I didn't think I should be concerned about iterating over a document with 4 <li>s. https://codereview.chromium.org/313363004/diff/40001/chrome/browser/resources... chrome/browser/resources/uber/uber_frame.js:91: if (lastSelectedNavItem) On 2014/06/06 21:21:23, Dan Beam wrote: > ^ it would seem that there might not be a selected nav item from this code The page starts out without a selected nav item. https://codereview.chromium.org/313363004/diff/40001/chrome/browser/resources... chrome/browser/resources/uber/uber_frame.js:101: var selectedNavItem = document.querySelector('li.selected'); On 2014/06/06 21:21:23, Dan Beam wrote: > nit: make a > > function getSelectedNavItem() { > return document.querySelector('li.selected'); > } > > a re-use here and other places Done. https://codereview.chromium.org/313363004/diff/40001/chrome/browser/resources... chrome/browser/resources/uber/uber_frame.js:101: var selectedNavItem = document.querySelector('li.selected'); On 2014/06/06 21:21:23, Dan Beam wrote: > what happens if this returns null? bad things.. but.. it can't because of the ordering in changeSelection. if the selected navItem didn't exist, setSelection would have thrown TypeError before this call. https://codereview.chromium.org/313363004/diff/40001/chrome/browser/resources... chrome/browser/resources/uber/uber_frame.js:105: selectedNavItem.getAttribute('group')); On 2014/06/06 21:21:23, Dan Beam wrote: > you don't ever actually use selectedNavItem itself, I'd just recommend > > var selectedNavItem = getSelectectedNavItem(); > if (!selectedNavItem) // or assert(selectedNavItem) > return; > > var selectedGroup = selectedNavItem.getAttribute('group'); > // use |selectedGroup| Done. https://codereview.chromium.org/313363004/diff/40001/chrome/browser/ui/settin... File chrome/browser/ui/settings_window_manager.h (right): https://codereview.chromium.org/313363004/diff/40001/chrome/browser/ui/settin... chrome/browser/ui/settings_window_manager.h:33: // Shows a chrome:// page (e.g. Settings, Help) in an an existing system On 2014/06/06 21:21:23, Dan Beam wrote: > ^ shouldn't this be About, now? ;) Done, thanks.
Ping - could you take another look today? We'd like to get it in by EOD. Thanks! Michael
LGTM
lgtm https://codereview.chromium.org/313363004/diff/80001/chrome/browser/resources... File chrome/browser/resources/uber/uber_frame.js (right): https://codereview.chromium.org/313363004/diff/80001/chrome/browser/resources... chrome/browser/resources/uber/uber_frame.js:84: * @return {Object} The currently selected nav item, if any. {Element} https://codereview.chromium.org/313363004/diff/80001/chrome/browser/resources... chrome/browser/resources/uber/uber_frame.js:112: for (var i = 0; i < navItems.length; ++i) nit: curlies https://codereview.chromium.org/313363004/diff/80001/chrome/browser/ui/webui/... File chrome/browser/ui/webui/uber/uber_ui.cc (right): https://codereview.chromium.org/313363004/diff/80001/chrome/browser/ui/webui/... chrome/browser/ui/webui/uber/uber_ui.cc:113: base::string16 settingsGroup(ASCIIToUTF16("settings_group")); cpp_vars_like_this
The CQ bit was checked by michaelpg@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/michaelpg@chromium.org/313363004/140001
https://codereview.chromium.org/313363004/diff/80001/chrome/browser/resources... File chrome/browser/resources/uber/uber_frame.js (right): https://codereview.chromium.org/313363004/diff/80001/chrome/browser/resources... chrome/browser/resources/uber/uber_frame.js:84: * @return {Object} The currently selected nav item, if any. On 2014/06/09 21:22:24, Dan Beam wrote: > {Element} Done. https://codereview.chromium.org/313363004/diff/80001/chrome/browser/resources... chrome/browser/resources/uber/uber_frame.js:112: for (var i = 0; i < navItems.length; ++i) On 2014/06/09 21:22:24, Dan Beam wrote: > nit: curlies Done. https://codereview.chromium.org/313363004/diff/80001/chrome/browser/ui/webui/... File chrome/browser/ui/webui/uber/uber_ui.cc (right): https://codereview.chromium.org/313363004/diff/80001/chrome/browser/ui/webui/... chrome/browser/ui/webui/uber/uber_ui.cc:113: base::string16 settingsGroup(ASCIIToUTF16("settings_group")); On 2014/06/09 21:22:24, Dan Beam wrote: > cpp_vars_like_this Whoops, missed this one. Done.
lgtm
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: android_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...)
The CQ bit was checked by michaelpg@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/michaelpg@chromium.org/313363004/140001
The CQ bit was checked by michaelpg@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/michaelpg@chromium.org/313363004/160001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: win_chromium_x64_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_x64_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)
The CQ bit was checked by michaelpg@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/michaelpg@chromium.org/313363004/160001
Message was sent while issue was closed.
Change committed as 276327 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
