Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1129513002:
Sad tab redesign for Windows, Linux and CrOS (Closed)
|
Created:
5 years, 7 months ago by edwardjung Modified:
5 years, 7 months ago CC:
chromium-reviews, oshima+watch_chromium.org, tfarina, mmccoy, bettes_chromium.org, ainslie Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionSad tab redesign for Windows, Linux and CrOS.
+ Update chrome://kill and chrome://crash icons. Both now use the same icon.
+ Make layout consistent with the SSL / Malware / net error set of interstitials.
+ Switched colours, margin, padding to use native UI constants.
BUG=457763
Committed: https://crrev.com/8898d3c15db0dc0d927d3fd7c7e15dc6f998167b
Cr-Commit-Position: refs/heads/master@{#330724}
Patch Set 1 #Patch Set 2 : Font corrections #
Total comments: 46
Patch Set 3 : Address review comments #Patch Set 4 : #
Total comments: 32
Patch Set 5 : Switch to native theme colours and standard layout margins #
Total comments: 10
Patch Set 6 : Fix nits #
Messages
Total messages: 29 (5 generated)
edwardjung@chromium.org changed reviewers: + msw@chromium.org
Mike, could you have a look at this update. We're just missing the final sad tab assets from the designer. But didn't want that to delay the review. I wasn't sure of how to handle the max width of and text labels. Whether I should alter the existing Label class or extend it. I end up extending it, but let me know if I should do something else. Thanks.
https://codereview.chromium.org/1129513002/diff/20001/chrome/browser/resource... File chrome/browser/resources/crashes.css (right): https://codereview.chromium.org/1129513002/diff/20001/chrome/browser/resource... chrome/browser/resources/crashes.css:15: background-position: 0 -20px; Why is this needed? How does Views use CSS? Will this affect Mac, etc.? https://codereview.chromium.org/1129513002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/sad_tab_view.cc (left): https://codereview.chromium.org/1129513002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/sad_tab_view.cc:103: (kind_ == chrome::SAD_TAB_KIND_CRASHED) ? IDR_SAD_TAB : IDR_KILLED_TAB)); Are we no longer using IDR_KILLED_TAB? Remove the image assets, enum, etc. https://codereview.chromium.org/1129513002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/sad_tab_view.cc (right): https://codereview.chromium.org/1129513002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/sad_tab_view.cc:42: const int kTopPadding = 100; Why are we doing such excessive padding instead of centering? https://codereview.chromium.org/1129513002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/sad_tab_view.cc:48: const SkColor kTextColor = SkColorSetRGB(100, 100, 100); This isn't cool. Can we use (or make these) native theme colors? https://codereview.chromium.org/1129513002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/sad_tab_view.cc:52: // const SkColor kButtonColor = SkColorSetRGB(76, 142, 250); Please review your own code thoroughly before asking others to do so. https://codereview.chromium.org/1129513002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/sad_tab_view.cc:61: class LabelWithMaxWidth : public Label { Can you instead call Label::SizeToFit for the multi-line labels (with the max of the desired kMaxContentWidth and the available container/tab width) and let the single line labels just use their preferred size. https://codereview.chromium.org/1129513002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/sad_tab_view.cc:70: int width = std::min(out.width(), kMaxContentWidth); Might any single-line labels go through this code path with translated text that exceeds this width? I worry that this width-capping isn't correct everywhere. https://codereview.chromium.org/1129513002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/sad_tab_view.cc:76: LabelWithMaxWidth::~LabelWithMaxWidth() {} nit: inline https://codereview.chromium.org/1129513002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/sad_tab_view.cc:119: // Linux and Windows font size discrepency indentation... https://codereview.chromium.org/1129513002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/sad_tab_view.cc:121: #if defined(OS_WIN) This is not acceptable. Pick a ui::ResourceBundle::MediumFont, SmallFont, BaseFont, LargeFont or whatever and just use that. Having consistent UI throughout our product is more important than matching mock perfectly. https://codereview.chromium.org/1129513002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/sad_tab_view.cc:141: columns->AddPaddingColumn(0.5, kPadding); You can leave both of these at 1 for evenly balanced re-sizing. https://codereview.chromium.org/1129513002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/sad_tab_view.cc:157: // Font size is too large on Linux. No, if that's really the case we need a fix to the font size enums, etc. https://codereview.chromium.org/1129513002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/sad_tab_view.cc:180: IDS_SAD_TAB_HELP_LINK : IDS_LEARN_MORE)); Do both mocks actually have a help link? https://codereview.chromium.org/1129513002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/sad_tab_view.cc:196: l10n_util::GetStringUTF16(IDS_SAD_TAB_HELP_LINK); Why is this different than |help_link_|'s text? Is this right for both pages? https://codereview.chromium.org/1129513002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/sad_tab_view.cc:227: views::GridLayout::TRAILING, kMaxContentWidth, kLabelHeight); Will longer translated text be able to wrap and use its needed height? https://codereview.chromium.org/1129513002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/sad_tab_view.cc:233: views::StyledLabel* feedback_link_ = Why is this not simply a views::Link? There is no ranged style here... If we need a multi-line link, it should be trivial to extend views::Link to support multiline via Label. https://codereview.chromium.org/1129513002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/sad_tab_view.cc:277: void SadTabView::StyledLabelLinkClicked(const gfx::Range& range, Ugh StyledLabel... This callback doesn't even specify what StyledLabel was clicked, it must be recorded elsewhere or assumed, and the gfx::Range is useless without knowing the text... I really want to destroy/rewrite StyledLabel. https://codereview.chromium.org/1129513002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/sad_tab_view.cc:279: Remove blank line. https://codereview.chromium.org/1129513002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/sad_tab_view.cc:280: // Will this work for i18n other languages in RTL for example? I doubt it... this does not seem acceptable to me. https://codereview.chromium.org/1129513002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/sad_tab_view.cc:282: GURL help_url((kind_ == chrome::SAD_TAB_KIND_CRASHED) ? Don't copy this code... refactor it to be used here and above. https://codereview.chromium.org/1129513002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/sad_tab_view.h (right): https://codereview.chromium.org/1129513002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/sad_tab_view.h:26: Remove blank lines... please review your own changes first.
Bettes, Ainslie, could you comment on the design related comments that Mike raised. > https://codereview.chromium.org/1129513002/diff/20001/chrome/browser/ui/views... > chrome/browser/ui/views/sad_tab_view.cc:42: const int kTopPadding = 100; > Why are we doing such excessive padding instead of centering? Referring to the 100px top padding versus centered. This was covered in the bug. > https://codereview.chromium.org/1129513002/diff/20001/chrome/browser/ui/views... > chrome/browser/ui/views/sad_tab_view.cc:48: const SkColor kTextColor = > SkColorSetRGB(100, 100, 100); > This isn't cool. Can we use (or make these) native theme colors? Thoughts on colours. The current implementation of the kill page looks like it uses a set of different colours from the native theme. > https://codereview.chromium.org/1129513002/diff/20001/chrome/browser/ui/views... > chrome/browser/ui/views/sad_tab_view.cc:121: #if defined(OS_WIN) > This is not acceptable. Pick a ui::ResourceBundle::MediumFont, SmallFont, > BaseFont, LargeFont or whatever and just use that. Having consistent UI > throughout our product is more important than matching mock perfectly. Should we use the standard font sizes only? In general we are trying to go for consistency with the other web UI interstitials. But I see that this wouldn't match the font sizes in other native UI places.
On 2015/05/12 15:45:10, edwardjung wrote: > Bettes, Ainslie, could you comment on the design related comments that Mike > raised. > > > > https://codereview.chromium.org/1129513002/diff/20001/chrome/browser/ui/views... > > chrome/browser/ui/views/sad_tab_view.cc:42: const int kTopPadding = 100; > > Why are we doing such excessive padding instead of centering? > > Referring to the 100px top padding versus centered. This was covered in the bug. > > > > https://codereview.chromium.org/1129513002/diff/20001/chrome/browser/ui/views... > > chrome/browser/ui/views/sad_tab_view.cc:48: const SkColor kTextColor = > > SkColorSetRGB(100, 100, 100); > > This isn't cool. Can we use (or make these) native theme colors? > > Thoughts on colours. The current implementation of the kill page looks like it > uses a set of different colours from the native theme. > > > > https://codereview.chromium.org/1129513002/diff/20001/chrome/browser/ui/views... > > chrome/browser/ui/views/sad_tab_view.cc:121: #if defined(OS_WIN) > > This is not acceptable. Pick a ui::ResourceBundle::MediumFont, SmallFont, > > BaseFont, LargeFont or whatever and just use that. Having consistent UI > > throughout our product is more important than matching mock perfectly. > > Should we use the standard font sizes only? In general we are trying to go for > consistency with the other web UI interstitials. But I see that this wouldn't > match the font sizes in other native UI places. Can you translate to me what Medium, Small, Base, and Large mean in terms of px/pts? What other web UI are we talking about?
Sorry I hit reply too early. See inline On 2015/05/12 15:45:10, edwardjung wrote: > Bettes, Ainslie, could you comment on the design related comments that Mike > raised. > > > > https://codereview.chromium.org/1129513002/diff/20001/chrome/browser/ui/views... > > chrome/browser/ui/views/sad_tab_view.cc:42: const int kTopPadding = 100; > > Why are we doing such excessive padding instead of centering? > > Referring to the 100px top padding versus centered. This was covered in the bug. This is just a reflection of bringing some of material ratios and guidelines to the desktop. Designing the layout so that its content is closer in proximity to the origin and omnibox was our goal. > > > > https://codereview.chromium.org/1129513002/diff/20001/chrome/browser/ui/views... > > chrome/browser/ui/views/sad_tab_view.cc:48: const SkColor kTextColor = > > SkColorSetRGB(100, 100, 100); > > This isn't cool. Can we use (or make these) native theme colors? > > Thoughts on colours. The current implementation of the kill page looks like it > uses a set of different colours from the native theme. It's hard to follow without screenshots. Are we referring to using the default grey, instead of the current dark blue? > > > > https://codereview.chromium.org/1129513002/diff/20001/chrome/browser/ui/views... > > chrome/browser/ui/views/sad_tab_view.cc:121: #if defined(OS_WIN) > > This is not acceptable. Pick a ui::ResourceBundle::MediumFont, SmallFont, > > BaseFont, LargeFont or whatever and just use that. Having consistent UI > > throughout our product is more important than matching mock perfectly. > > Should we use the standard font sizes only? In general we are trying to go for > consistency with the other web UI interstitials. But I see that this wouldn't > match the font sizes in other native UI places. Can you translate to me what Medium, Small, Base, and Large mean in terms of px/pts? What other web UI are we talking about?
> Can you translate to me what Medium, Small, Base, and Large mean in terms of > px/pts? What other web UI are we talking about? I'm not sure of the base font size on windows but it's 12px on Linux const int kSmallFontSizeDelta = -1; const int kMediumFontSizeDelta = 3; const int kLargeFontSizeDelta = 8; > What other web UI are we talking about? I was referring to being consistent with the net error / SSL / malware web ui interstitials.
On 2015/05/12 17:01:03, edwardjung wrote: > > Can you translate to me what Medium, Small, Base, and Large mean in terms of > > px/pts? What other web UI are we talking about? > > I'm not sure of the base font size on windows but it's 12px on Linux > > const int kSmallFontSizeDelta = -1; > const int kMediumFontSizeDelta = 3; > const int kLargeFontSizeDelta = 8; > So what I'm seeing on https://ianfette.org is outside the norms of what we normally do for text? I > > What other web UI are we talking about? > I was referring to being consistent with the net error / SSL / malware web ui > interstitials. Oh right. You're saying that it's consistent with what we've built on other interstitials but not so much native ui. So is ://settings using BaseFont for their content, and LargeFont for their headers?
On 2015/05/12 17:07:49, bettes wrote: > On 2015/05/12 17:01:03, edwardjung wrote: > > > Can you translate to me what Medium, Small, Base, and Large mean in terms of > > > px/pts? What other web UI are we talking about? > > > > I'm not sure of the base font size on windows but it's 12px on Linux > > > > const int kSmallFontSizeDelta = -1; > > const int kMediumFontSizeDelta = 3; > > const int kLargeFontSizeDelta = 8; > > > > So what I'm seeing on https://ianfette.org is outside the norms of what we > normally do for text? I I'm not entirely sure what the are norms for the web UI. But those interstitials were all built per the spec. > > > > What other web UI are we talking about? > > I was referring to being consistent with the net error / SSL / malware web ui > > interstitials. > > Oh right. You're saying that it's consistent with what we've built on other > interstitials but not so much native ui. So is ://settings using BaseFont for > their content, and LargeFont for their headers? Settings is also built in web UI. I haven't checked all the font sizes, but the header is 18px so that doesn't match to any of the defined native UI sizes. Mike would be able to give you a proper explanation of the native UI fonts. But native UI fonts would take into account your OS level font settings and preferences I assume.
On 2015/05/12 17:26:02, edwardjung wrote: > On 2015/05/12 17:07:49, bettes wrote: > > On 2015/05/12 17:01:03, edwardjung wrote: > > > > Can you translate to me what Medium, Small, Base, and Large mean in terms > of > > > > px/pts? What other web UI are we talking about? > > > > > > I'm not sure of the base font size on windows but it's 12px on Linux > > > > > > const int kSmallFontSizeDelta = -1; > > > const int kMediumFontSizeDelta = 3; > > > const int kLargeFontSizeDelta = 8; > > > > > > > So what I'm seeing on https://ianfette.org is outside the norms of what we > > normally do for text? I > > I'm not entirely sure what the are norms for the web UI. But those interstitials > were all built per the spec. > > > > > > > What other web UI are we talking about? > > > I was referring to being consistent with the net error / SSL / malware web > ui > > > interstitials. > > > > Oh right. You're saying that it's consistent with what we've built on other > > interstitials but not so much native ui. So is ://settings using BaseFont for > > their content, and LargeFont for their headers? > > Settings is also built in web UI. I haven't checked all the font sizes, but the > header is 18px so that doesn't match to any of the defined native UI sizes. > > Mike would be able to give you a proper explanation of the native UI fonts. But > native UI fonts would take into account your OS level font settings and > preferences I assume. Well being that interstitials are an extension of the Chrome voice, just as all other Web UI properties, I don't see the issue in not complying with Os level standards. As far as I'm concerned, we're following suit with what is already established in our other UI.
Please respond to inline comments with inline comments, not here. On 2015/05/12 15:45:10, edwardjung wrote: > > Why are we doing such excessive padding instead of centering? > Referring to the 100px top padding versus centered. This was covered in the bug. Shouldn't we shrink the 100px vertical whitespace if the container is small? (do we really want a fixed 100px top margin for a small window?) > > This isn't cool. Can we use (or make these) native theme colors? > Thoughts on colours. The current implementation of the kill page looks like it > uses a set of different colours from the native theme. That's true, but the new mocks look like themed window and text colors. If we are converging on style, let's be consistent with our dialogs/windows. (so when dialog and window colors change, they're changed everywhere) Try kColorId_DialogBackground or kColorId_WindowBackground. > > This is not acceptable. Pick a ui::ResourceBundle::MediumFont, SmallFont, > > BaseFont, LargeFont or whatever and just use that. Having consistent UI > > throughout our product is more important than matching mock perfectly. > > Should we use the standard font sizes only? In general we are trying to go for > consistency with the other web UI interstitials. But I see that this wouldn't > match the font sizes in other native UI places. My inclination is to go with the native font sizes for native font rendering. Our browser UI code shouldn't try to wrangle fixed font size values like this. If the user has a larger/smaller system font size, the code is wrong anyway.
> Please respond to inline comments with inline comments, not here. Sorry, that's my bad. Bettes and ainslie don't know the code review tool so felt it was easier for them to comment generally rather than going through the actual code.
Updated to address your comments, now using the base font sizes and using relative vertical padding. The font / background colours are consistent with the SSL and net error interstitial UIs. Thanks. https://codereview.chromium.org/1129513002/diff/20001/chrome/browser/resource... File chrome/browser/resources/crashes.css (right): https://codereview.chromium.org/1129513002/diff/20001/chrome/browser/resource... chrome/browser/resources/crashes.css:15: background-position: 0 -20px; On 2015/05/11 18:27:08, msw wrote: > Why is this needed? How does Views use CSS? Will this affect Mac, etc.? The supplied updated icon has padding which makes the positioning off on the chrome://crashes page. This is a fix for that. However as I'm getting new cropped icons this isn't required. https://codereview.chromium.org/1129513002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/sad_tab_view.cc (left): https://codereview.chromium.org/1129513002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/sad_tab_view.cc:103: (kind_ == chrome::SAD_TAB_KIND_CRASHED) ? IDR_SAD_TAB : IDR_KILLED_TAB)); On 2015/05/11 18:27:09, msw wrote: > Are we no longer using IDR_KILLED_TAB? Remove the image assets, enum, etc. That's correct, they will use the same image. I'll remove. https://codereview.chromium.org/1129513002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/sad_tab_view.cc (right): https://codereview.chromium.org/1129513002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/sad_tab_view.cc:42: const int kTopPadding = 100; On 2015/05/11 18:27:09, msw wrote: > Why are we doing such excessive padding instead of centering? The design was updated to be vertically anchored to the top with a maximum top padding. This makes it consistent with other interstitial pages. The bug thread has the updated design. As you indicated, on smaller screen sizes the padding should be reduced and this is indeed how the other interstitials work. I've altered this to go for a top weighted layout which more closely matches the design whilst remaining somewhat responsive to the screen size. https://codereview.chromium.org/1129513002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/sad_tab_view.cc:48: const SkColor kTextColor = SkColorSetRGB(100, 100, 100); On 2015/05/11 18:27:09, msw wrote: > This isn't cool. Can we use (or make these) native theme colors Per the chat with ainslie and bettes to not use the native theme colours for dialogues and windows, as we are treating the interstitials as a set which should be consistent and not determined by the OS level theme. I did try the colours suggested kColorId_DialogBackground or kColorId_WindowBackground but they came out as black. https://codereview.chromium.org/1129513002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/sad_tab_view.cc:52: // const SkColor kButtonColor = SkColorSetRGB(76, 142, 250); On 2015/05/11 18:27:10, msw wrote: > Please review your own code thoroughly before asking others to do so. Apologies again. https://codereview.chromium.org/1129513002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/sad_tab_view.cc:61: class LabelWithMaxWidth : public Label { On 2015/05/11 18:27:09, msw wrote: > Can you instead call Label::SizeToFit for the multi-line labels (with the max of > the desired kMaxContentWidth and the available container/tab width) and let the > single line labels just use their preferred size. Thanks for the suggestion. I've updated SadTabView::Layout which calls SizeToFit. Removed this class. https://codereview.chromium.org/1129513002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/sad_tab_view.cc:70: int width = std::min(out.width(), kMaxContentWidth); On 2015/05/11 18:27:09, msw wrote: > Might any single-line labels go through this code path with translated text that > exceeds this width? I worry that this width-capping isn't correct everywhere. I removed this class per suggestion above. https://codereview.chromium.org/1129513002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/sad_tab_view.cc:76: LabelWithMaxWidth::~LabelWithMaxWidth() {} On 2015/05/11 18:27:09, msw wrote: > nit: inline No longer applicable https://codereview.chromium.org/1129513002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/sad_tab_view.cc:119: // Linux and Windows font size discrepency On 2015/05/11 18:27:09, msw wrote: > indentation... Done. https://codereview.chromium.org/1129513002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/sad_tab_view.cc:121: #if defined(OS_WIN) On 2015/05/11 18:27:09, msw wrote: > This is not acceptable. Pick a ui::ResourceBundle::MediumFont, SmallFont, > BaseFont, LargeFont or whatever and just use that. Having consistent UI > throughout our product is more important than matching mock perfectly. In our minds, we are trying to achieve consistency with the other interstitials UIs. However we agreed to use the standard platform font sizes for the accessibility. BaseFont is an acceptable size. https://codereview.chromium.org/1129513002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/sad_tab_view.cc:141: columns->AddPaddingColumn(0.5, kPadding); On 2015/05/11 18:27:09, msw wrote: > You can leave both of these at 1 for evenly balanced re-sizing. Done. https://codereview.chromium.org/1129513002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/sad_tab_view.cc:157: // Font size is too large on Linux. On 2015/05/11 18:27:09, msw wrote: > No, if that's really the case we need a fix to the font size enums, etc. Removed the font adjustment https://codereview.chromium.org/1129513002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/sad_tab_view.cc:180: IDS_SAD_TAB_HELP_LINK : IDS_LEARN_MORE)); On 2015/05/11 18:27:09, msw wrote: > Do both mocks actually have a help link? You're correct, only chrome://kill has the help link. Moved to the right section. https://codereview.chromium.org/1129513002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/sad_tab_view.cc:196: l10n_util::GetStringUTF16(IDS_SAD_TAB_HELP_LINK); On 2015/05/11 18:27:08, msw wrote: > Why is this different than |help_link_|'s text? Is this right for both pages? This is the suggestion link which only appears on chrome://kill ainslie is suggesting some new text, so this may get consolidated. https://codereview.chromium.org/1129513002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/sad_tab_view.cc:227: views::GridLayout::TRAILING, kMaxContentWidth, kLabelHeight); On 2015/05/11 18:27:10, msw wrote: > Will longer translated text be able to wrap and use its needed height? You're right, the height isn't enough here for longer text at the most narrow viewport. Changed to setting the height via SizeToFit() in SadTabView::Layout(). https://codereview.chromium.org/1129513002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/sad_tab_view.cc:233: views::StyledLabel* feedback_link_ = On 2015/05/11 18:27:08, msw wrote: > Why is this not simply a views::Link? There is no ranged style here... > If we need a multi-line link, it should be trivial to extend views::Link to > support multiline via Label. I'll do as suggested. https://codereview.chromium.org/1129513002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/sad_tab_view.cc:279: On 2015/05/11 18:27:09, msw wrote: > Remove blank line. Done. https://codereview.chromium.org/1129513002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/sad_tab_view.cc:280: // Will this work for i18n other languages in RTL for example? On 2015/05/11 18:27:09, msw wrote: > I doubt it... this does not seem acceptable to me. Removed this check as there is now only a single instance of StyledLabel. https://codereview.chromium.org/1129513002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/sad_tab_view.cc:282: GURL help_url((kind_ == chrome::SAD_TAB_KIND_CRASHED) ? On 2015/05/11 18:27:09, msw wrote: > Don't copy this code... refactor it to be used here and above. Done. https://codereview.chromium.org/1129513002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/sad_tab_view.h (right): https://codereview.chromium.org/1129513002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/sad_tab_view.h:26: On 2015/05/11 18:27:10, msw wrote: > Remove blank lines... please review your own changes first. Done. Sorry I should be more careful.
https://codereview.chromium.org/1129513002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/sad_tab_view.cc (right): https://codereview.chromium.org/1129513002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/sad_tab_view.cc:48: const SkColor kTextColor = SkColorSetRGB(100, 100, 100); On 2015/05/14 18:31:26, edwardjung wrote: > On 2015/05/11 18:27:09, msw wrote: > > This isn't cool. Can we use (or make these) native theme colors > > Per the chat with ainslie and bettes to not use the native theme colours for > dialogues and windows, as we are treating the interstitials as a set which > should be consistent and not determined by the OS level theme. > > I did try the colours suggested kColorId_DialogBackground or > kColorId_WindowBackground but they came out as black. Post screenshots as I suggest, ainslie and bettes might not object. If the background was black, the enum usage wasn't correct. https://codereview.chromium.org/1129513002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/sad_tab_view.cc:233: views::StyledLabel* feedback_link_ = On 2015/05/14 18:31:27, edwardjung wrote: > On 2015/05/11 18:27:08, msw wrote: > > Why is this not simply a views::Link? There is no ranged style here... > > If we need a multi-line link, it should be trivial to extend views::Link to > > support multiline via Label. > > I'll do as suggested. Did you try this? https://codereview.chromium.org/1129513002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/sad_tab_view.cc:282: GURL help_url((kind_ == chrome::SAD_TAB_KIND_CRASHED) ? On 2015/05/14 18:31:26, edwardjung wrote: > On 2015/05/11 18:27:09, msw wrote: > > Don't copy this code... refactor it to be used here and above. > > Done. Not done, afaict. https://codereview.chromium.org/1129513002/diff/60001/chrome/app/theme/theme_... File chrome/app/theme/theme_resources.grd (left): https://codereview.chromium.org/1129513002/diff/60001/chrome/app/theme/theme_... chrome/app/theme/theme_resources.grd:438: <structure type="chrome_scaled_image" name="IDR_KILLED_TAB" file="common/killtab.png" /> Please git rm chrome/app/theme/default_100_percent/common/killtab.png https://codereview.chromium.org/1129513002/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/sad_tab_view.cc (right): https://codereview.chromium.org/1129513002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/sad_tab_view.cc:12: #include "chrome/browser/browser_process.h" Is this needed? https://codereview.chromium.org/1129513002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/sad_tab_view.cc:32: #include "ui/views/controls/scroll_view.h" Is this needed? https://codereview.chromium.org/1129513002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/sad_tab_view.cc:42: const int kPadding = 24; Find corresponding (or close enough) ui/views/layout/layout_constants.h values. Magic numbers are generally discouraged. https://codereview.chromium.org/1129513002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/sad_tab_view.cc:49: const SkColor kTextColor = SkColorSetRGB(100, 100, 100); Let's see a screenshot with these colors versus my suggested themed text and window colors. I suggest using the default label color instead of kTitleColor, using the default color or maybe kColorId_LabelDisabledColor instead of kTextColor, and using kColorId_DialogBackground instead of kBackgroundColor. https://codereview.chromium.org/1129513002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/sad_tab_view.cc:65: title_(NULL) { Init the other new members too and use nullptr instead throughout. https://codereview.chromium.org/1129513002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/sad_tab_view.cc:98: gfx::FontList body_font_list_ = rb.GetFontList(ui::ResourceBundle::BaseFont); Don't use the underscore postfix for stack locals. This probably isn't needed at all, BaseFont is the default. https://codereview.chromium.org/1129513002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/sad_tab_view.cc:123: (kind_ == chrome::SAD_TAB_KIND_CRASHED) ? IDS_SAD_TAB_TITLE nit: maybe cache a local const bool crashed for here and elsewhere. https://codereview.chromium.org/1129513002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/sad_tab_view.cc:126: title_->SetMultiLine(true); I wonder if "Aw Snap" or "He's Dead Jim" really need to wrap. https://codereview.chromium.org/1129513002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/sad_tab_view.cc:149: reload_button_->SetMinSize(gfx::Size(90, 36)); What does the button look like if this isn't specified? https://codereview.chromium.org/1129513002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/sad_tab_view.cc:173: views::StyledLabel::RangeStyleInfo::CreateForLink(); This doesn't seem correct, why use CreateForLink for the non-link default style? https://codereview.chromium.org/1129513002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/sad_tab_view.cc:183: gfx::Range(help_prefix.length(), help_message_string.length() - 1), Using |help_message_string.length() - 1| is incorrect. I think this assumes the untranslated/unlocalized text, which just ends in a period. This should be |help_prefix.length() + link_text.length()|. https://codereview.chromium.org/1129513002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/sad_tab_view.cc:236: GURL help_url(chrome::kCrashReasonURL); As I said in my earlier comment... Don't copy code, refactor to share the code, or call LinkClicked(help_link_) https://codereview.chromium.org/1129513002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/sad_tab_view.cc:251: int messageWidth = width() - kPadding * 2; nit: use the unix_hacker naming convention. https://codereview.chromium.org/1129513002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/sad_tab_view.cc:252: int width = messageWidth > kMaxContentWidth ? kMaxContentWidth : messageWidth; Just do const int width = std::min(width() - kPadding * 2, kMaxContentWidth) https://codereview.chromium.org/1129513002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/sad_tab_view.cc:256: if (kind_ == chrome::SAD_TAB_KIND_KILLED) { Just check if |feedback_link_| and |help_message_| are not nullptr instead of checking for the |kind_|.
Thanks for baring with me on this update. I've posted screenshots of the updated implementation using the native colours: https://code.google.com/p/chromium/issues/detail?id=457763#c21 https://codereview.chromium.org/1129513002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/sad_tab_view.cc (right): https://codereview.chromium.org/1129513002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/sad_tab_view.cc:48: const SkColor kTextColor = SkColorSetRGB(100, 100, 100); On 2015/05/14 23:50:19, msw wrote: > On 2015/05/14 18:31:26, edwardjung wrote: > > On 2015/05/11 18:27:09, msw wrote: > > > This isn't cool. Can we use (or make these) native theme colors > > > > Per the chat with ainslie and bettes to not use the native theme colours for > > dialogues and windows, as we are treating the interstitials as a set which > > should be consistent and not determined by the OS level theme. > > > > I did try the colours suggested kColorId_DialogBackground or > > kColorId_WindowBackground but they came out as black. > > Post screenshots as I suggest, ainslie and bettes might not object. > If the background was black, the enum usage wasn't correct. You assumed correct, I wasn't passing the enum to the getSystemColor call. https://codereview.chromium.org/1129513002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/sad_tab_view.cc:282: GURL help_url((kind_ == chrome::SAD_TAB_KIND_CRASHED) ? On 2015/05/14 23:50:19, msw wrote: > On 2015/05/14 18:31:26, edwardjung wrote: > > On 2015/05/11 18:27:09, msw wrote: > > > Don't copy this code... refactor it to be used here and above. > > > > Done. > > Not done, afaict. Misunderstood. https://codereview.chromium.org/1129513002/diff/60001/chrome/app/theme/theme_... File chrome/app/theme/theme_resources.grd (left): https://codereview.chromium.org/1129513002/diff/60001/chrome/app/theme/theme_... chrome/app/theme/theme_resources.grd:438: <structure type="chrome_scaled_image" name="IDR_KILLED_TAB" file="common/killtab.png" /> On 2015/05/14 23:50:19, msw wrote: > Please git rm chrome/app/theme/default_100_percent/common/killtab.png Done. https://codereview.chromium.org/1129513002/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/sad_tab_view.cc (right): https://codereview.chromium.org/1129513002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/sad_tab_view.cc:12: #include "chrome/browser/browser_process.h" On 2015/05/14 23:50:20, msw wrote: > Is this needed? Removed https://codereview.chromium.org/1129513002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/sad_tab_view.cc:32: #include "ui/views/controls/scroll_view.h" On 2015/05/14 23:50:20, msw wrote: > Is this needed? Removed. Had experimented with having a scroll view for the content area. https://codereview.chromium.org/1129513002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/sad_tab_view.cc:42: const int kPadding = 24; On 2015/05/14 23:50:20, msw wrote: > Find corresponding (or close enough) ui/views/layout/layout_constants.h values. > Magic numbers are generally discouraged. Switched to the layout constants. https://codereview.chromium.org/1129513002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/sad_tab_view.cc:65: title_(NULL) { On 2015/05/14 23:50:19, msw wrote: > Init the other new members too and use nullptr instead throughout. Done. https://codereview.chromium.org/1129513002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/sad_tab_view.cc:98: gfx::FontList body_font_list_ = rb.GetFontList(ui::ResourceBundle::BaseFont); On 2015/05/14 23:50:20, msw wrote: > Don't use the underscore postfix for stack locals. > This probably isn't needed at all, BaseFont is the default. Removed. https://codereview.chromium.org/1129513002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/sad_tab_view.cc:123: (kind_ == chrome::SAD_TAB_KIND_CRASHED) ? IDS_SAD_TAB_TITLE On 2015/05/14 23:50:20, msw wrote: > nit: maybe cache a local const bool crashed for here and elsewhere. Done. https://codereview.chromium.org/1129513002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/sad_tab_view.cc:126: title_->SetMultiLine(true); On 2015/05/14 23:50:20, msw wrote: > I wonder if "Aw Snap" or "He's Dead Jim" really need to wrap. In my testing of Russian and Arabic, He's Dead Jim is pretty long and would need wrapping at the narrowest window width. https://codereview.chromium.org/1129513002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/sad_tab_view.cc:149: reload_button_->SetMinSize(gfx::Size(90, 36)); On 2015/05/14 23:50:20, msw wrote: > What does the button look like if this isn't specified? Looks fine. Remove. https://codereview.chromium.org/1129513002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/sad_tab_view.cc:173: views::StyledLabel::RangeStyleInfo::CreateForLink(); On 2015/05/14 23:50:20, msw wrote: > This doesn't seem correct, why use CreateForLink for the non-link default style? Fixed. https://codereview.chromium.org/1129513002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/sad_tab_view.cc:183: gfx::Range(help_prefix.length(), help_message_string.length() - 1), On 2015/05/14 23:50:19, msw wrote: > Using |help_message_string.length() - 1| is incorrect. I think this assumes the > untranslated/unlocalized text, which just ends in a period. This should be > |help_prefix.length() + link_text.length()|. Good point. Not all languages will have a punctuation at the end. https://codereview.chromium.org/1129513002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/sad_tab_view.cc:236: GURL help_url(chrome::kCrashReasonURL); On 2015/05/14 23:50:19, msw wrote: > As I said in my earlier comment... Don't copy code, refactor to share the code, > or call LinkClicked(help_link_) Switched to call LinkClicked. https://codereview.chromium.org/1129513002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/sad_tab_view.cc:251: int messageWidth = width() - kPadding * 2; On 2015/05/14 23:50:20, msw wrote: > nit: use the unix_hacker naming convention. Done. https://codereview.chromium.org/1129513002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/sad_tab_view.cc:252: int width = messageWidth > kMaxContentWidth ? kMaxContentWidth : messageWidth; On 2015/05/14 23:50:19, msw wrote: > Just do const int width = std::min(width() - kPadding * 2, kMaxContentWidth) Done. https://codereview.chromium.org/1129513002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/sad_tab_view.cc:256: if (kind_ == chrome::SAD_TAB_KIND_KILLED) { On 2015/05/14 23:50:20, msw wrote: > Just check if |feedback_link_| and |help_message_| are not nullptr instead of > checking for the |kind_|. Done.
This is looking pretty good; can you post about:crash pics? https://codereview.chromium.org/1129513002/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/sad_tab_view.cc (right): https://codereview.chromium.org/1129513002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/sad_tab_view.cc:126: title_->SetMultiLine(true); On 2015/05/18 12:02:36, edwardjung wrote: > On 2015/05/14 23:50:20, msw wrote: > > I wonder if "Aw Snap" or "He's Dead Jim" really need to wrap. > > In my testing of Russian and Arabic, He's Dead Jim is pretty long and would need > wrapping at the narrowest window width. Acknowledged. https://codereview.chromium.org/1129513002/diff/80001/chrome/app/theme/theme_... File chrome/app/theme/theme_resources.grd (left): https://codereview.chromium.org/1129513002/diff/80001/chrome/app/theme/theme_... chrome/app/theme/theme_resources.grd:438: <structure type="chrome_scaled_image" name="IDR_KILLED_TAB" file="common/killtab.png" /> Does Mac (or Android or iOS) still use IDR_KILLED_TAB? I don't see references to it on codesearch, I just don't want to break any non-views chrome:kill page. https://codereview.chromium.org/1129513002/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/sad_tab_view.cc (right): https://codereview.chromium.org/1129513002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/sad_tab_view.cc:88: ui::ResourceBundle& rb = ui::ResourceBundle::GetSharedInstance(); nit: move this down to where it's used. https://codereview.chromium.org/1129513002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/sad_tab_view.cc:110: layout->AddPaddingRow(2, views::kPanelVerticalSpacing); q: I'm not totally sure how these |vertical_resize| arguments are used, but maybe this could be 1 and the call below could be 2 (instead of 2 here and 4 below), if they're weights for how the excess padding is distributed. https://codereview.chromium.org/1129513002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/sad_tab_view.cc:245: if (feedback_link_ != nullptr) { nit: remove curly braces. https://codereview.chromium.org/1129513002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/sad_tab_view.cc:247: } else if (help_message_ != nullptr) { nit: maybe don't bother with the else here, it's not inherent that these members are exclusive.
Updated with nit fixes. chrome://crash screenshots posted here: https://code.google.com/p/chromium/issues/detail?id=457763#c23 https://codereview.chromium.org/1129513002/diff/80001/chrome/app/theme/theme_... File chrome/app/theme/theme_resources.grd (left): https://codereview.chromium.org/1129513002/diff/80001/chrome/app/theme/theme_... chrome/app/theme/theme_resources.grd:438: <structure type="chrome_scaled_image" name="IDR_KILLED_TAB" file="common/killtab.png" /> On 2015/05/18 19:24:50, msw wrote: > Does Mac (or Android or iOS) still use IDR_KILLED_TAB? I don't see references to > it on codesearch, I just don't want to break any non-views chrome:kill page. I don't believe so. - On Android, chrome://kill returns the chrome://crash which uses IDR_SAD_TAB. The clank version is currently being updated by mmccoy. - Mac uses IDR_SAD_TAB in chrome/browser/ui/cocoa/tab_contents/sad_tab_view_cocoa.mm - iOS doesn't show the kill / crash page. https://codereview.chromium.org/1129513002/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/sad_tab_view.cc (right): https://codereview.chromium.org/1129513002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/sad_tab_view.cc:88: ui::ResourceBundle& rb = ui::ResourceBundle::GetSharedInstance(); On 2015/05/18 19:24:51, msw wrote: > nit: move this down to where it's used. Done. https://codereview.chromium.org/1129513002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/sad_tab_view.cc:110: layout->AddPaddingRow(2, views::kPanelVerticalSpacing); It's meant to be a percentage, but yes 1, 2 does seem to equal 2,4. https://codereview.chromium.org/1129513002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/sad_tab_view.cc:245: if (feedback_link_ != nullptr) { On 2015/05/18 19:24:50, msw wrote: > nit: remove curly braces. Done. https://codereview.chromium.org/1129513002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/sad_tab_view.cc:247: } else if (help_message_ != nullptr) { On 2015/05/18 19:24:50, msw wrote: > nit: maybe don't bother with the else here, it's not inherent that these members > are exclusive. Done.
lgtm, but please expand the CL description, and I'd like to see some screenshots with short browser windows (I'd like to see how the vertical layout/padding handles windows with a barely sufficient height and an insufficient height).
edwardjung@chromium.org changed reviewers: + oshima@chromium.org
Thanks, small screen screenshots: https://code.google.com/p/chromium/issues/detail?id=457763#c24 Oshima, could you take a look at the updated resources in chrome/app/theme/* Thanks.
c/a/theme lgtm
The CQ bit was checked by edwardjung@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1129513002/100001
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/1129513002/100001
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/8898d3c15db0dc0d927d3fd7c7e15dc6f998167b Cr-Commit-Position: refs/heads/master@{#330724} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
