Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1182493009:
Wrench menu reorg phase 2 (Closed)
|
Created:
5 years, 6 months ago by edwardjung Modified:
5 years, 1 month ago CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionWrench menu reorg phase 2
+ Move History into the recent tabs submenu. Renamed submenu to 'History and Recent tabs'
+ Move 'About Chrome' under the Help submenu. Renamed to 'Help and About'
+ Moved 'Saved page as' under 'More tools'.
+ Removed the 'View source', 'JS Console' and 'Inspect devices' menu items. Note a link to 'Inspect devices' will be surfaced in dev tools.
BUG=432561
Committed: https://crrev.com/6d69201744d8ddeb6f8a5bac956015aadd084d3f
Cr-Commit-Position: refs/heads/master@{#336113}
Patch Set 1 #Patch Set 2 : Update RecentTabsSubMenuModel unit tests #Patch Set 3 : Fix WrenchMenuModel unit test #Patch Set 4 : Fix Mac wrenchmenu unit test #Patch Set 5 : Mac unit test fix #
Total comments: 1
Patch Set 6 : Fix incorrect title case expr causing wrong strings to be displayed on OSX #
Total comments: 40
Patch Set 7 : Address review comments #Patch Set 8 : #Patch Set 9 : Address unit test comment #Patch Set 10 : Reinstate 'About' menu item to CrOS #
Total comments: 10
Patch Set 11 : Fix nits #Patch Set 12 : Fix merge error #Messages
Total messages: 32 (8 generated)
edwardjung@chromium.org changed reviewers: + pkasting@chromium.org, rsesek@chromium.org
Hi, Please take a look at this wrench menu update: rsesek - chrome/browser/ui/cocoa/wrench_menu/wrench_menu_controller_unittest.mm pkasting - chrome/browser/ui/toolbar/* Thanks. https://codereview.chromium.org/1182493009/diff/80001/chrome/app/generated_re... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/1182493009/diff/80001/chrome/app/generated_re... chrome/app/generated_resources.grd:1357: + More Too&ls Fixed the incorrect capitalisation.
cocoa/ LGTM
https://codereview.chromium.org/1182493009/diff/100001/chrome/app/generated_r... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/1182493009/diff/100001/chrome/app/generated_r... chrome/app/generated_resources.grd:1357: + More Too&ls Lowercase T https://codereview.chromium.org/1182493009/diff/100001/chrome/app/generated_r... chrome/app/generated_resources.grd:1580: + H&elp and About Lowercase A https://codereview.chromium.org/1182493009/diff/100001/chrome/app/generated_r... chrome/app/generated_resources.grd:12385: + History and Recent tabs Lowercase R https://codereview.chromium.org/1182493009/diff/100001/chrome/browser/ui/tool... File chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc (right): https://codereview.chromium.org/1182493009/diff/100001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc:74: const int kLocalEntriesStartIndex = 1; This seems like a technically-inaccurate name given that these entries actually start at 2, not 1. https://codereview.chromium.org/1182493009/diff/100001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc:216: IDC_SHOW_HISTORY, &show_history_accelerator_); This block duplicates the one just below https://codereview.chromium.org/1182493009/diff/100001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc:258: } else if (command_id == IDC_SHOW_HISTORY) { Nit: No else after return https://codereview.chromium.org/1182493009/diff/100001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc:395: // Add the History item to top of the menu followed by a separator. Nit: This sentence is redundant with the comments above + code below, I'd remove it https://codereview.chromium.org/1182493009/diff/100001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc:398: Nit: I'd also remove this blank line https://codereview.chromium.org/1182493009/diff/100001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc:405: // Always starts at the 2 second item in the list. These comments don't make much sense, plus they seem likely to get out of date. The whole reason you defined a constant for this was so you wouldn't have to give exact values elsewhere. https://codereview.chromium.org/1182493009/diff/100001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc:406: last_local_model_index_ = kLocalEntriesStartIndex; Nit: Do this right above the DCHECK https://codereview.chromium.org/1182493009/diff/100001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc:415: DCHECK_EQ(last_local_model_index_, 1); Nit: (expected, actual) Though, I don't see why you're DCHECKing this value. This defeats the purpose of using a separate constant for this. https://codereview.chromium.org/1182493009/diff/100001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc:426: Nit: Why did you add this? https://codereview.chromium.org/1182493009/diff/100001/chrome/browser/ui/tool... File chrome/browser/ui/toolbar/recent_tabs_sub_menu_model_unittest.cc (right): https://codereview.chromium.org/1182493009/diff/100001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/recent_tabs_sub_menu_model_unittest.cc:237: EXPECT_TRUE(model.GetLabelFontListAt(2) != NULL); Nit: nullptr (several places) https://codereview.chromium.org/1182493009/diff/100001/chrome/browser/ui/tool... File chrome/browser/ui/toolbar/wrench_menu_model.cc (left): https://codereview.chromium.org/1182493009/diff/100001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/wrench_menu_model.cc:318: AddItemWithStringId(IDC_VIEW_SOURCE, IDS_VIEW_SOURCE); Why are we removing View Source? I don't think that makes sense. https://codereview.chromium.org/1182493009/diff/100001/chrome/browser/ui/tool... File chrome/browser/ui/toolbar/wrench_menu_model.cc (right): https://codereview.chromium.org/1182493009/diff/100001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/wrench_menu_model.cc:251: // Don't display the about menu item on Chrome OS. Nit: Say why not. https://codereview.chromium.org/1182493009/diff/100001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/wrench_menu_model.cc:323: Nit: I can't figure out the logic of when this function adds blank lines. Before separators? After? Everywhere? Nowhere? None of these seems to accurately describe it. Pick a system and stick with it. https://codereview.chromium.org/1182493009/diff/100001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/wrench_menu_model.cc:929: // AddItemWithStringId(IDC_SAVE_PAGE, IDS_SAVE_PAGE); In or out, but not in-but-commented-out. https://codereview.chromium.org/1182493009/diff/100001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/wrench_menu_model.cc:965: // Display the about menu item for non Chrome OS and non Google builds. Why do we have two blocks in this file both dedicated to adding this ABOUT item? https://codereview.chromium.org/1182493009/diff/100001/chrome/browser/ui/tool... File chrome/browser/ui/toolbar/wrench_menu_model_unittest.cc (right): https://codereview.chromium.org/1182493009/diff/100001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/wrench_menu_model_unittest.cc:158: // Bookmarks submenu. Nit: "The bookmarks submenu comes after the Tabs and Downloads items."
Thanks for the quick review Peter. Updated to address your comments. https://codereview.chromium.org/1182493009/diff/100001/chrome/app/generated_r... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/1182493009/diff/100001/chrome/app/generated_r... chrome/app/generated_resources.grd:1357: + More Too&ls On 2015/06/17 22:59:42, Peter Kasting wrote: > Lowercase T This is the title case section. I've added this to the description. https://codereview.chromium.org/1182493009/diff/100001/chrome/app/generated_r... chrome/app/generated_resources.grd:1580: + H&elp and About On 2015/06/17 22:59:42, Peter Kasting wrote: > Lowercase A Done. https://codereview.chromium.org/1182493009/diff/100001/chrome/app/generated_r... chrome/app/generated_resources.grd:12385: + History and Recent tabs On 2015/06/17 22:59:42, Peter Kasting wrote: > Lowercase R Done. https://codereview.chromium.org/1182493009/diff/100001/chrome/browser/ui/tool... File chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc (right): https://codereview.chromium.org/1182493009/diff/100001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc:74: const int kLocalEntriesStartIndex = 1; On 2015/06/17 22:59:42, Peter Kasting wrote: > This seems like a technically-inaccurate name given that these entries actually > start at 2, not 1. Renamed. https://codereview.chromium.org/1182493009/diff/100001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc:216: IDC_SHOW_HISTORY, &show_history_accelerator_); On 2015/06/17 22:59:42, Peter Kasting wrote: > This block duplicates the one just below Removed. https://codereview.chromium.org/1182493009/diff/100001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc:258: } else if (command_id == IDC_SHOW_HISTORY) { On 2015/06/17 22:59:42, Peter Kasting wrote: > Nit: No else after return Done. https://codereview.chromium.org/1182493009/diff/100001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc:395: // Add the History item to top of the menu followed by a separator. On 2015/06/17 22:59:42, Peter Kasting wrote: > Nit: This sentence is redundant with the comments above + code below, I'd remove > it Done. https://codereview.chromium.org/1182493009/diff/100001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc:398: On 2015/06/17 22:59:42, Peter Kasting wrote: > Nit: I'd also remove this blank line Done. https://codereview.chromium.org/1182493009/diff/100001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc:405: // Always starts at the 2 second item in the list. On 2015/06/17 22:59:42, Peter Kasting wrote: > These comments don't make much sense, plus they seem likely to get out of date. > The whole reason you defined a constant for this was so you wouldn't have to > give exact values elsewhere. Removed. https://codereview.chromium.org/1182493009/diff/100001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc:406: last_local_model_index_ = kLocalEntriesStartIndex; On 2015/06/17 22:59:42, Peter Kasting wrote: > Nit: Do this right above the DCHECK Done. https://codereview.chromium.org/1182493009/diff/100001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc:415: DCHECK_EQ(last_local_model_index_, 1); On 2015/06/17 22:59:42, Peter Kasting wrote: > Nit: (expected, actual) > > Though, I don't see why you're DCHECKing this value. This defeats the purpose > of using a separate constant for this. Good point, removed. https://codereview.chromium.org/1182493009/diff/100001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc:426: On 2015/06/17 22:59:42, Peter Kasting wrote: > Nit: Why did you add this? Removed. https://codereview.chromium.org/1182493009/diff/100001/chrome/browser/ui/tool... File chrome/browser/ui/toolbar/recent_tabs_sub_menu_model_unittest.cc (right): https://codereview.chromium.org/1182493009/diff/100001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/recent_tabs_sub_menu_model_unittest.cc:237: EXPECT_TRUE(model.GetLabelFontListAt(2) != NULL); On 2015/06/17 22:59:42, Peter Kasting wrote: > Nit: nullptr (several places) Fixed. https://codereview.chromium.org/1182493009/diff/100001/chrome/browser/ui/tool... File chrome/browser/ui/toolbar/wrench_menu_model.cc (left): https://codereview.chromium.org/1182493009/diff/100001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/wrench_menu_model.cc:318: AddItemWithStringId(IDC_VIEW_SOURCE, IDS_VIEW_SOURCE); > Why are we removing View Source? I don't think that makes sense. There's a lot more detail reorganisation in the bug thread but an overview: We're trying to encourage Dev Tools as the entry point for viewing the source, console and devices. For Linux and Mac users, the OS level menu still has the view source item. On all platforms view source is available from right clicking on a page and the shortcut key. Most developers would use dev tools or know the right click / shortcut. Also the total usage of view source in the wrench menu and it was very small percentage in the overall view source usage and wrench menu clicks. Current 7 day view source UMA stats for all browsers: 3.32% of total View Source views came from the wrench menu item (54,208) View source clicks accounts for 0.073% of all wrench menu items. On Windows: 3.41% of total view source visits (45,785). 0.067% of total menu clicks. https://codereview.chromium.org/1182493009/diff/100001/chrome/browser/ui/tool... File chrome/browser/ui/toolbar/wrench_menu_model.cc (right): https://codereview.chromium.org/1182493009/diff/100001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/wrench_menu_model.cc:251: // Don't display the about menu item on Chrome OS. On 2015/06/17 22:59:43, Peter Kasting wrote: > Nit: Say why not. Done. Although there doesn't seem to a logical reason not to have a direct link to about. I've asked around our team if it should be reinstated which would simplify the code. https://codereview.chromium.org/1182493009/diff/100001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/wrench_menu_model.cc:323: On 2015/06/17 22:59:43, Peter Kasting wrote: > Nit: I can't figure out the logic of when this function adds blank lines. > Before separators? After? Everywhere? Nowhere? None of these seems to > accurately describe it. Pick a system and stick with it. Reformatted. Blank lines added for each section of the menu structure described in the function comment. Also reformatted WrenchMenuModel::Build in the same manner. https://codereview.chromium.org/1182493009/diff/100001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/wrench_menu_model.cc:929: // AddItemWithStringId(IDC_SAVE_PAGE, IDS_SAVE_PAGE); On 2015/06/17 22:59:43, Peter Kasting wrote: > In or out, but not in-but-commented-out. Done. https://codereview.chromium.org/1182493009/diff/100001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/wrench_menu_model.cc:965: // Display the about menu item for non Chrome OS and non Google builds. On 2015/06/17 22:59:43, Peter Kasting wrote: > Why do we have two blocks in this file both dedicated to adding this ABOUT item? We moved the 'About' menu item under the help submenu (block above). However on Chromium builds the help submenu is not displayed so reinstate the menu item at the top level. I've added an extra comment to describe this. https://codereview.chromium.org/1182493009/diff/100001/chrome/browser/ui/tool... File chrome/browser/ui/toolbar/wrench_menu_model_unittest.cc (right): https://codereview.chromium.org/1182493009/diff/100001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/wrench_menu_model_unittest.cc:158: // Bookmarks submenu. On 2015/06/17 22:59:43, Peter Kasting wrote: > Nit: "The bookmarks submenu comes after the Tabs and Downloads items." Done.
https://codereview.chromium.org/1182493009/diff/100001/chrome/browser/ui/tool... File chrome/browser/ui/toolbar/wrench_menu_model.cc (left): https://codereview.chromium.org/1182493009/diff/100001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/wrench_menu_model.cc:318: AddItemWithStringId(IDC_VIEW_SOURCE, IDS_VIEW_SOURCE); On 2015/06/18 16:52:05, edwardjung wrote: > > Why are we removing View Source? I don't think that makes sense. > > There's a lot more detail reorganisation in the bug thread but an overview: > > We're trying to encourage Dev Tools as the entry point for viewing the source, > console and devices. For Linux and Mac users, the OS level menu still has the > view source item. On all platforms view source is available from right clicking > on a page and the shortcut key. Most developers would use dev tools or know the > right click / shortcut. > > Also the total usage of view source in the wrench menu and it was very small > percentage in the overall view source usage and wrench menu clicks. > > Current 7 day view source UMA stats for all browsers: > 3.32% of total View Source views came from the wrench menu item (54,208) > View source clicks accounts for 0.073% of all wrench menu items. > > On Windows: > 3.41% of total view source visits (45,785). > 0.067% of total menu clicks. I'm still concerned about this. * 3.5% of total View Source visits still seems like a reasonably high amount. * The Dev Tools don't make it easy for novices like me to figure out how to "view source", which I do actually need to do on a surprising number of occasions. I worry about this being disproportionately difficult for non-developers. I think we still want View Source to be easily accessible for such users. * Basic CHI principles argue against making a context menu option the only or primary way of exposing a feature. * View Source has a great deal of nostalgic and PR value. Many people view it as the capability which democratized the web. Removing it from these menus could hurt us on these fronts. I'm normally a big fan of simplifying and streamlining menus, but given that this is already buried in a submenu, I'm gravely concerned that the potential costs of removing this particular item outweigh the benefits. I'd like to not do this change. In the limit, I'd like to make it as a completely separate, experiment-driven change, and collect sufficient stats to see whether users in the experimental group figure out how to substitute another way of getting to this, or whether overall usage decreases. If usage decreases, that seems like an argument against the change.
ainslie@, Peter has some concerns regarding the removal of the 'view source' menu item. I agree that it's not easy to navigate dev tools to view the actual source versus the rendered DOM, I don't know if there could be improvements there. The UI of dev tools is going through some change down the line but don't know if the sources panel will get a make over. I also do still use view source a lot as a lighter and quicker dev tools to for quick checks, so also agree that for novices who don't know the shortcut, on Windows / CrOS at least, we can't educate them once the menu item is removed.
https://codereview.chromium.org/1182493009/diff/100001/chrome/browser/ui/tool... File chrome/browser/ui/toolbar/wrench_menu_model.cc (left): https://codereview.chromium.org/1182493009/diff/100001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/wrench_menu_model.cc:318: AddItemWithStringId(IDC_VIEW_SOURCE, IDS_VIEW_SOURCE); On 2015/06/18 17:16:58, Peter Kasting wrote: > On 2015/06/18 16:52:05, edwardjung wrote: > > > Why are we removing View Source? I don't think that makes sense. > > > > There's a lot more detail reorganisation in the bug thread but an overview: > > > > We're trying to encourage Dev Tools as the entry point for viewing the source, > > console and devices. For Linux and Mac users, the OS level menu still has the > > view source item. On all platforms view source is available from right > clicking > > on a page and the shortcut key. Most developers would use dev tools or know > the > > right click / shortcut. > > > > Also the total usage of view source in the wrench menu and it was very small > > percentage in the overall view source usage and wrench menu clicks. > > > > Current 7 day view source UMA stats for all browsers: > > 3.32% of total View Source views came from the wrench menu item (54,208) > > View source clicks accounts for 0.073% of all wrench menu items. > > > > On Windows: > > 3.41% of total view source visits (45,785). > > 0.067% of total menu clicks. > > I'm still concerned about this. > > * 3.5% of total View Source visits still seems like a reasonably high amount. > * The Dev Tools don't make it easy for novices like me to figure out how to > "view source", which I do actually need to do on a surprising number of > occasions. I worry about this being disproportionately difficult for > non-developers. I think we still want View Source to be easily accessible for > such users. > * Basic CHI principles argue against making a context menu option the only or > primary way of exposing a feature. > * View Source has a great deal of nostalgic and PR value. Many people view it > as the capability which democratized the web. Removing it from these menus > could hurt us on these fronts. > > I'm normally a big fan of simplifying and streamlining menus, but given that > this is already buried in a submenu, I'm gravely concerned that the potential > costs of removing this particular item outweigh the benefits. I'd like to not > do this change. In the limit, I'd like to make it as a completely separate, > experiment-driven change, and collect sufficient stats to see whether users in > the experimental group figure out how to substitute another way of getting to > this, or whether overall usage decreases. If usage decreases, that seems like > an argument against the change. Copying comments here so that ainslie can see the thread. I agree that it's not easy to navigate dev tools to view the actual source versus the rendered DOM, I don't know if there could be improvements there. The UI of dev tools is going through some change down the line but don't know if the sources panel will get a make over. I also do still use view source a lot as a lighter and quicker dev tools to for quick checks, so also agree that for novices who don't know the shortcut, on Windows / CrOS at least, we can't educate them once the menu item is removed.
Hmm. - Items that receive so few clicks (0.067% of menu clicks) don't belong in the menu. - We aren't removing view source, we're just routing people who need it through the core DevTools access point. PR risk seems low. - We're also not leaving the developers who use the view-source menu item without a path forward because the "developer tools" menu item will show up in the same region spatially that was previously home to view-source. - The DevTools team is already preparing for the menu simplification - so maybe we could suggest to them some extra keyboard shortcut user education as part of their future work (since it'll also affect other views - notably js-console and inspect-devices) https://code.google.com/p/chromium/issues/detail?id=499734 - Another option for the transition would be to temporarily use a more verbose string (ex: "JS, Source, and Developer tools") as we're doing for the other merging sections Happy to chat more if you have other questions/suggestions (though I'm WFSF tomorrow so won't be back in MTV until Monday).
On 2015/06/18 23:34:44, ainslie wrote: > Hmm. > > - Items that receive so few clicks (0.067% of menu clicks) don't belong in the > menu. > - We aren't removing view source, we're just routing people who need it through > the core DevTools access point. PR risk seems low. As I said, I can't figure out where View Source is in the Dev Tools. I looked. Twice. I'm concerned that if I can't figure it out others will have problems too. There's a Sources pane, it doesn't seem to show the source of the current page and I don't know how to make it work. I'm sure there's a way to do this, but that way is not at all as simple as "there's a big View Source option in the front of the Dev Tools window". If there were a blindlingly obvious way to do this I'd still be uncomfortable, but less so. > - We're also not leaving the developers who use the view-source menu item > without a path forward because the "developer tools" menu item will show up in > the same region spatially that was previously home to view-source. The sorts of people who used View Source as non-developers (again, like me) aren't necessarily going to realize that they can find it in there. I wouldn't have thought of it. Maybe I'm extrapolating too much from a sample size of me, but I'm reluctant to assume that most people are _more_ able to find things in Chrome than I am. > - The DevTools team is already preparing for the menu simplification - so maybe > we could suggest to them some extra keyboard shortcut user education as part of > their future work (since it'll also affect other views - notably js-console and > inspect-devices) https://code.google.com/p/chromium/issues/detail?id=499734 > - Another option for the transition would be to temporarily use a more verbose > string (ex: "JS, Source, and Developer tools") as we're doing for the other > merging sections Ugly, but it would probably work if Dev Tools made View Source a big top-level item. In all of this, though, I keep coming back to: is the gain really worth it? We're talking about optimizing away one item that's already buried inside a rarely-used submenu. And the submenu is not very long, so I'm reluctant to believe users are overwhelmed by choices here. Again, I normally am on the side of simplification for things like this. But it feels like you're fighting this battle on principle, when in practice it would be safer to just leave this. I'm not asking for exceptions for any other items :)
I'd like to stick with the original plan that was approved by UI-Review back in March. We can try this on dev/beta/canary and watch the product forums and stats in the coming weeks. There will be a few teams watching closely for user confusion (conops, devtools, and devrel) and we've already communicated with them. Cost/benefit calculations for removing access points are always tricky because it's hard to reason about marginal cost of additional items. In this case, we're only removing one of many paths to the same functionality and we're guiding people toward the alternative tool that's a strict superset of functionality. (future thought: I know devtools folks keep tab choice sticky but we could ask them to consider defaulting into sources view - as an alternative to the big-button approach)
On 2015/06/19 13:39:44, ainslie wrote: > I'd like to stick with the original plan that was approved by UI-Review back in > March. We can try this on dev/beta/canary and watch the product forums and stats > in the coming weeks. There will be a few teams watching closely for user > confusion (conops, devtools, and devrel) and we've already communicated with > them. Can you at least go with the experiment-gated route I suggest? That will make it much easier to see concrete stats for this. > Cost/benefit calculations for removing access points are always tricky because > it's hard to reason about marginal cost of additional items. In this case, we're > only removing one of many paths to the same functionality and we're guiding > people toward the alternative tool that's a strict superset of functionality. I feel like a broken record, but there are only two other paths to this functionality (a context menu and the sources pane) and the sources pane you keep insisting is the right substitute does not show the page source by default and is non-obvious how to use. I've said this several times but you've never even acknowledged that one of the Chrome UI engineers couldn't immediately figure out how to do what you're saying is easy. If you want people to be able to use this, the dev tools team really needs to make changes, e.g. default to sources view if there is no most recent view for this page + default that view to showing the source of the current page instead of showing nothing.
Hey Peter - I know that the process of simplification is going to reduce the visibility of some features that have limited but non-zero usage. View Source is a good example - I use it myself. But ultimately the point of Project Eraser (and other related simplification efforts right now) is to start making these trade-offs more proactively rather than letting the status quo have a large inertial advantage. The right place for this functionality to live is among the dev tools. If we have concerns with the UI of the dev tools we should raise them to the dev tools team - and I'm happy to help do so. But we shouldn't solve problems in the dev tools with excess menu items even if we happen to have solved it that way in the past. For the vast majority of users the view source tool is just a potential misclick pushing other more viable options further away from the starting point of the cursor. This is a win for those many, many users. We shouldn't block this change based on the concern that users who are savvy enough to want to view the source code of a website aren't savvy enough to find the source code in the developer tools. If that concern proves problematic when we release this change, we can reconsider, but my instinct is that it will be small in practice. Happy to chat about this more in person if it's helpful. Cheers, T On 2015/06/19 18:50:47, Peter Kasting wrote: > On 2015/06/19 13:39:44, ainslie wrote: > > I'd like to stick with the original plan that was approved by UI-Review back > in > > March. We can try this on dev/beta/canary and watch the product forums and > stats > > in the coming weeks. There will be a few teams watching closely for user > > confusion (conops, devtools, and devrel) and we've already communicated with > > them. > > Can you at least go with the experiment-gated route I suggest? That will make > it much easier to see concrete stats for this. > > > Cost/benefit calculations for removing access points are always tricky because > > it's hard to reason about marginal cost of additional items. In this case, > we're > > only removing one of many paths to the same functionality and we're guiding > > people toward the alternative tool that's a strict superset of functionality. > > I feel like a broken record, but there are only two other paths to this > functionality (a context menu and the sources pane) and the sources pane you > keep insisting is the right substitute does not show the page source by default > and is non-obvious how to use. I've said this several times but you've never > even acknowledged that one of the Chrome UI engineers couldn't immediately > figure out how to do what you're saying is easy. > > If you want people to be able to use this, the dev tools team really needs to > make changes, e.g. default to sources view if there is no most recent view for > this page + default that view to showing the source of the current page instead > of showing nothing.
LGTM https://codereview.chromium.org/1182493009/diff/180001/chrome/app/generated_r... File chrome/app/generated_resources.grd (left): https://codereview.chromium.org/1182493009/diff/180001/chrome/app/generated_r... chrome/app/generated_resources.grd:1620: <message name="IDS_HELP_MENU" desc="The text label of the Help sub-menu item"> Nit: If this is the title case one, maybe add "In Title Case:" to the description here too. https://codereview.chromium.org/1182493009/diff/180001/chrome/app/generated_r... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/1182493009/diff/180001/chrome/app/generated_r... chrome/app/generated_resources.grd:12380: + No Tabs From Other Devices This is a descriptive (placeholder) string rather than a selectable entry, right? Do we still want to make it title case? I would have thought this wasn't title case but maybe I'm wrong. https://codereview.chromium.org/1182493009/diff/180001/chrome/browser/ui/tool... File chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc (right): https://codereview.chromium.org/1182493009/diff/180001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc:396: InsertSeparatorAt(1, ui::NORMAL_SEPARATOR); Can we ever have no items from both the below calls? If so, having a separator here would be wrong. (Of course, without it, we're down to just one item in this submenu, in which case we probably shouldn't have the submenu at all, and we should replace it with a top-level "History" item, maybe...) https://codereview.chromium.org/1182493009/diff/180001/chrome/browser/ui/tool... File chrome/browser/ui/toolbar/wrench_menu_model.cc (right): https://codereview.chromium.org/1182493009/diff/180001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/wrench_menu_model.cc:277: // More tools submenu is organised as follows: Tiny nit: I think in Chrome comments we generally use U.S. spellings of words (so "organized")? Or you can avoid debate by using a different word like "constructed" :) https://codereview.chromium.org/1182493009/diff/180001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/wrench_menu_model.cc:281: // - Profiling enabled option. Nit: "Option to enable profiling"?
Thanks again for reviewing Peter and bearing with us with regard the View source. I too am hoping there are changes to the Dev Tools sources panel to make it easier to navigate and get the source. https://codereview.chromium.org/1182493009/diff/180001/chrome/app/generated_r... File chrome/app/generated_resources.grd (left): https://codereview.chromium.org/1182493009/diff/180001/chrome/app/generated_r... chrome/app/generated_resources.grd:1620: <message name="IDS_HELP_MENU" desc="The text label of the Help sub-menu item"> On 2015/06/22 23:14:24, Peter Kasting wrote: > Nit: If this is the title case one, maybe add "In Title Case:" to the > description here too. Done. https://codereview.chromium.org/1182493009/diff/180001/chrome/app/generated_r... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/1182493009/diff/180001/chrome/app/generated_r... chrome/app/generated_resources.grd:12380: + No Tabs From Other Devices On 2015/06/22 23:14:24, Peter Kasting wrote: > This is a descriptive (placeholder) string rather than a selectable entry, > right? Do we still want to make it title case? I would have thought this > wasn't title case but maybe I'm wrong. That's right it's a placeholder and it does look a little odd in title case. However on OSX if you look under the battery icon you'll see a number of status type messages which are all in title case: 'Battery Is Charged', 'Power Source: Power Adapter'. So we would follow this. https://codereview.chromium.org/1182493009/diff/180001/chrome/browser/ui/tool... File chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc (right): https://codereview.chromium.org/1182493009/diff/180001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc:396: InsertSeparatorAt(1, ui::NORMAL_SEPARATOR); On 2015/06/22 23:14:24, Peter Kasting wrote: > Can we ever have no items from both the below calls? If so, having a separator > here would be wrong. (Of course, without it, we're down to just one item in > this submenu, in which case we probably shouldn't have the submenu at all, and > we should replace it with a top-level "History" item, maybe...) No items in both cases would result in the following minimum menu items: - Recently closed (disabled, showing shortcut for user education) - Separator - No tabs from other devices (disabled) https://codereview.chromium.org/1182493009/diff/180001/chrome/browser/ui/tool... File chrome/browser/ui/toolbar/wrench_menu_model.cc (right): https://codereview.chromium.org/1182493009/diff/180001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/wrench_menu_model.cc:277: // More tools submenu is organised as follows: On 2015/06/22 23:14:24, Peter Kasting wrote: > Tiny nit: I think in Chrome comments we generally use U.S. spellings of words > (so "organized")? Or you can avoid debate by using a different word like > "constructed" :) Americanise all the comments :D https://codereview.chromium.org/1182493009/diff/180001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/wrench_menu_model.cc:281: // - Profiling enabled option. On 2015/06/22 23:14:24, Peter Kasting wrote: > Nit: "Option to enable profiling"? Done.
The CQ bit was checked by edwardjung@chromium.org to run a CQ dry run
The patchset sent to the CQ was uploaded after l-g-t-m from rsesek@chromium.org, pkasting@chromium.org Link to the patchset: https://codereview.chromium.org/1182493009/#ps200001 (title: "Fix nits")
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1182493009/200001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Exceeded global retry quota
The CQ bit was checked by edwardjung@chromium.org to run a CQ dry run
The patchset sent to the CQ was uploaded after l-g-t-m from rsesek@chromium.org, pkasting@chromium.org Link to the patchset: https://codereview.chromium.org/1182493009/#ps220001 (title: "Fix merge error")
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1182493009/220001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by edwardjung@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1182493009/220001
Message was sent while issue was closed.
Committed patchset #12 (id:220001)
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/6d69201744d8ddeb6f8a5bac956015aadd084d3f Cr-Commit-Position: refs/heads/master@{#336113}
Message was sent while issue was closed.
On 2015/06/18 17:37:34, edwardjung wrote: > ainslie@, Peter has some concerns regarding the removal of the 'view source' > menu item. > > I agree that it's not easy to navigate dev tools to view the actual source > versus the rendered DOM, I don't know if there could be improvements there. The > UI of dev tools is going through some change down the line but don't know if the > sources panel will get a make over. > > I also do still use view source a lot as a lighter and quicker dev tools to for > quick checks, so also agree that for novices who don't know the shortcut, on > Windows / CrOS at least, we can't educate them once the menu item is removed. View Source was a quick and easy to use menu item for folks who aren't developers. Developer Tools is way more complicated than is needed by people who use View Source. I understand decisions have been made but I wanted to add the point of view of me and the people on the thread below. View Source Missing in Chrome 45 https://productforums.google.com/forum/?utm_medium=email&utm_source=first_pos...
Message was sent while issue was closed.
On 2015/10/06 17:53:25, debra_evans00 wrote: > View Source was a quick and easy to use menu item for folks who aren't > developers. Developer Tools is way more complicated than is needed by people > who use View Source. I understand decisions have been made but I wanted to add > the point of view of me and the people on the thread below. > > View Source Missing in Chrome 45 > https://productforums.google.com/forum/?utm_medium=email&utm_source=first_pos... I agree. This is very much a retrograde step. I appreciate it's also still available via the context menu, but this can be disabled via Javascript. In fact, one of the most common use cases of the menu item is when the context menu has been disabled. I'd really like to see this back. Even as a developer, it's a much quicker and simpler way of looking at the source of a page than Developer Tools. Losing this functionality for the sake of one line saved in a menu really doesn't seem like a sensible decision.
Message was sent while issue was closed.
> I appreciate it's also still available via the context menu, but this can be > disabled via Javascript. In fact, one of the most common use cases of the menu > item is when the context menu has been disabled. > I'd really like to see this back. Even as a developer, it's a much quicker and > simpler way of looking at the source of a page than Developer Tools. Losing this > functionality for the sake of one line saved in a menu really doesn't seem like > a sensible decision. Mark, thanks for the feedback. I don't have metrics but assume that it's a relatively small number of sites that actively disable context clicking. If they do you can use the shortcut ctrl+alt+U / cmd+alt+U to get to the view source page if the page has disabled the context menu. This also has the bonus of being quicker than any menu. On OSX and Linux the View Source item is still available under View > Developer in the OS level menu. |
