Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
195283003:
Adds a hide button to the desktop capture notification bar. (Closed)
|
Created:
6 years, 9 months ago by jiayl Modified:
6 years, 9 months ago CC:
chromium-reviews, tfarina, vrk (LEFT CHROMIUM), juberti2 Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionAdds a hide button to the desktop capture notification bar.
The new button is used to minimize the notification to the taskbar (or Mac Dock). It's designed to look like a link to differentiate itself from the "stop sharing" button, which is an action on desktop capture.
BUG=316731
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=256851
Patch Set 1 #
Total comments: 5
Patch Set 2 : fix window icon not showing in Windows #
Total comments: 1
Patch Set 3 : add comment #
Total comments: 5
Patch Set 4 : #
Total comments: 7
Patch Set 5 : remove new string #
Total comments: 1
Patch Set 6 : #Patch Set 7 : sync #
Messages
Total messages: 38 (0 generated)
PTAL. We've run this through security and UI team and are trying to merge it to M34.
On 2014/03/11 22:51:39, jiayl wrote: > PTAL. We've run this through security and UI team and are trying to merge it to > M34. More info: https://docs.google.com/a/google.com/presentation/d/1VnCdaXpO7Hdfrj8SY0-rY_mt...
https://codereview.chromium.org/195283003/diff/1/chrome/browser/ui/views/scre... File chrome/browser/ui/views/screen_capture_notification_ui_views.cc (right): https://codereview.chromium.org/195283003/diff/1/chrome/browser/ui/views/scre... chrome/browser/ui/views/screen_capture_notification_ui_views.cc:210: gfx::Size hide_link_size = child_at(3)->GetPreferredSize(); Please replace child_at(?) here with label_, stop_button_ and hide_link_ https://codereview.chromium.org/195283003/diff/1/chrome/browser/ui/views/scre... chrome/browser/ui/views/screen_capture_notification_ui_views.cc:297: GetWidget()->Minimize(); Does this work as expected on Windows and Linux? Normally the notification window should not be shown in the task bar, so I'm not sure where it goes when you minimize it.
https://codereview.chromium.org/195283003/diff/1/chrome/browser/ui/views/scre... File chrome/browser/ui/views/screen_capture_notification_ui_views.cc (right): https://codereview.chromium.org/195283003/diff/1/chrome/browser/ui/views/scre... chrome/browser/ui/views/screen_capture_notification_ui_views.cc:210: gfx::Size hide_link_size = child_at(3)->GetPreferredSize(); On 2014/03/11 23:45:38, Sergey Ulanov wrote: > Please replace child_at(?) here with label_, stop_button_ and hide_link_ Done. https://codereview.chromium.org/195283003/diff/1/chrome/browser/ui/views/scre... chrome/browser/ui/views/screen_capture_notification_ui_views.cc:297: GetWidget()->Minimize(); On 2014/03/11 23:45:38, Sergey Ulanov wrote: > Does this work as expected on Windows and Linux? Normally the notification > window should not be shown in the task bar, so I'm not sure where it goes when > you minimize it. It works as expected on Linux because the notification window takes a separate icon on the taskbar. I just realized it does not work well on Windows because the notification bar does not occupy a space on the taskbar. Sergey/Ben, how can I make the notification bar to show in the taskbar on Windows? force_show_in_taskbar does not seem to have any effect.
cocoa/ lgtm
PTAL https://codereview.chromium.org/195283003/diff/1/chrome/browser/ui/views/scre... File chrome/browser/ui/views/screen_capture_notification_ui_views.cc (right): https://codereview.chromium.org/195283003/diff/1/chrome/browser/ui/views/scre... chrome/browser/ui/views/screen_capture_notification_ui_views.cc:297: GetWidget()->Minimize(); Fixed now. Confirmed working on Linux/Windows/Mac. On 2014/03/11 23:54:35, jiayl wrote: > On 2014/03/11 23:45:38, Sergey Ulanov wrote: > > Does this work as expected on Windows and Linux? Normally the notification > > window should not be shown in the task bar, so I'm not sure where it goes when > > you minimize it. > It works as expected on Linux because the notification window takes a separate > icon on the taskbar. > > I just realized it does not work well on Windows because the notification bar > does not occupy a space on the taskbar. > > Sergey/Ben, how can I make the notification bar to show in the taskbar on > Windows? force_show_in_taskbar does not seem to have any effect.
ben/msw, please review the ui/views change.
lgtm https://codereview.chromium.org/195283003/diff/40001/chrome/browser/ui/views/... File chrome/browser/ui/views/screen_capture_notification_ui_views.cc (right): https://codereview.chromium.org/195283003/diff/40001/chrome/browser/ui/views/... chrome/browser/ui/views/screen_capture_notification_ui_views.cc:293: return false; Add comment to explain why we need to override this method.
Why doesn't this view hide itself after a delay like the fullscreen_exit_bubble? Was the plan to add a hide link run by UX? Why does it use a link instead of a button that would match the stop button? Can you post screenshots of before/after appearances on Mac and some Views build (Windows, ChromeOS, or Linux Aura)? https://codereview.chromium.org/195283003/diff/60001/chrome/browser/ui/views/... File chrome/browser/ui/views/screen_capture_notification_ui_views.cc (right): https://codereview.chromium.org/195283003/diff/60001/chrome/browser/ui/views/... chrome/browser/ui/views/screen_capture_notification_ui_views.cc:224: grip_rect.set_y(bounds().height() / 2 - grip_rect.height() / 2); nit: do grip_rect.set_y(bounds().height() - grip_rect.height()) / 2); as below. https://codereview.chromium.org/195283003/diff/60001/chrome/browser/ui/views/... chrome/browser/ui/views/screen_capture_notification_ui_views.cc:230: stop_button_rect.set_x(bounds().width() - stop_button_rect.width() - nit: Could you use a BoxLayout for this view? Otherwise, it seems like it'd be simpler to layout |hide_link_| before |stop_button_|, like this: hide_link_rect.set_x(bounds().width() - hide_link_rect.width()); ... stop_button_rect.set_x(hide_link_rect.x() - stop_button_rect.width() - kHorizontalMargin); https://codereview.chromium.org/195283003/diff/60001/chrome/browser/ui/views/... chrome/browser/ui/views/screen_capture_notification_ui_views.cc:238: gfx::Rect label_rect(label_->GetPreferredSize()); nit: |label_|'s preferred size is not used at all, why bother making this call? https://codereview.chromium.org/195283003/diff/60001/chrome/browser/ui/views/... chrome/browser/ui/views/screen_capture_notification_ui_views.cc:247: stop_button_rect.width() + kHorizontalMargin + hide_link_rect.width(), nit: consider hide_link_rect.right() - stop_button_rect.x() https://codereview.chromium.org/195283003/diff/60001/chrome/browser/ui/views/... chrome/browser/ui/views/screen_capture_notification_ui_views.cc:304: views::Link* source, int event_flags) { nit: one parameter per line.
Please see https://docs.google.com/a/google.com/presentation/d/1VnCdaXpO7Hdfrj8SY0-rY_mt... for background and screenshots. The security team and UX team have given green light for this change. We considered many different options (including the one like fullscreen_exit_bubble) and chose the current design for its simplicity in implementation and clearness in UX. This is only a short term solution for M34 to unblock a product launch and we need the code change to be small enough for a merge. As to why 'hide' is a link instead of a button: 'hide' is an action on the notification window, while 'stop sharing' is an action on desktop capturing. So we want 'hide' to be a different UI element than "stop sharing" to avoid confusion. We can chat in person if you have more questions. And nits resolved. On 2014/03/12 03:58:13, msw wrote: > Why doesn't this view hide itself after a delay like the fullscreen_exit_bubble? > Was the plan to add a hide link run by UX? Why does it use a link instead of a > button that would match the stop button? Can you post screenshots of > before/after appearances on Mac and some Views build (Windows, ChromeOS, or > Linux Aura)? > > https://codereview.chromium.org/195283003/diff/60001/chrome/browser/ui/views/... > File chrome/browser/ui/views/screen_capture_notification_ui_views.cc (right): > > https://codereview.chromium.org/195283003/diff/60001/chrome/browser/ui/views/... > chrome/browser/ui/views/screen_capture_notification_ui_views.cc:224: > grip_rect.set_y(bounds().height() / 2 - grip_rect.height() / 2); > nit: do grip_rect.set_y(bounds().height() - grip_rect.height()) / 2); as below. > > https://codereview.chromium.org/195283003/diff/60001/chrome/browser/ui/views/... > chrome/browser/ui/views/screen_capture_notification_ui_views.cc:230: > stop_button_rect.set_x(bounds().width() - stop_button_rect.width() - > nit: Could you use a BoxLayout for this view? Otherwise, it seems like it'd be > simpler to layout |hide_link_| before |stop_button_|, like this: > hide_link_rect.set_x(bounds().width() - hide_link_rect.width()); > ... > stop_button_rect.set_x(hide_link_rect.x() - stop_button_rect.width() - > kHorizontalMargin); > > https://codereview.chromium.org/195283003/diff/60001/chrome/browser/ui/views/... > chrome/browser/ui/views/screen_capture_notification_ui_views.cc:238: gfx::Rect > label_rect(label_->GetPreferredSize()); > nit: |label_|'s preferred size is not used at all, why bother making this call? > > https://codereview.chromium.org/195283003/diff/60001/chrome/browser/ui/views/... > chrome/browser/ui/views/screen_capture_notification_ui_views.cc:247: > stop_button_rect.width() + kHorizontalMargin + hide_link_rect.width(), > nit: consider hide_link_rect.right() - stop_button_rect.x() > > https://codereview.chromium.org/195283003/diff/60001/chrome/browser/ui/views/... > chrome/browser/ui/views/screen_capture_notification_ui_views.cc:304: > views::Link* source, int event_flags) { > nit: one parameter per line.
On 2014/03/12 16:41:44, jiayl wrote: > Please see > https://docs.google.com/a/google.com/presentation/d/1VnCdaXpO7Hdfrj8SY0-rY_mt... > for background and screenshots. > > The security team and UX team have given green light for this change. > > We considered many different options (including the one like > fullscreen_exit_bubble) and chose the current design for its simplicity in > implementation and clearness in UX. This is only a short term solution for M34 > to unblock a product launch and we need the code change to be small enough for a > merge. > > As to why 'hide' is a link instead of a button: 'hide' is an action on the > notification window, while 'stop sharing' is an action on desktop capturing. So > we want 'hide' to be a different UI element than "stop sharing" to avoid > confusion. > > We can chat in person if you have more questions. > > And nits resolved. > On 2014/03/12 03:58:13, msw wrote: > > Why doesn't this view hide itself after a delay like the > fullscreen_exit_bubble? > > Was the plan to add a hide link run by UX? Why does it use a link instead of a > > button that would match the stop button? Can you post screenshots of > > before/after appearances on Mac and some Views build (Windows, ChromeOS, or > > Linux Aura)? > > > > > https://codereview.chromium.org/195283003/diff/60001/chrome/browser/ui/views/... > > File chrome/browser/ui/views/screen_capture_notification_ui_views.cc (right): > > > > > https://codereview.chromium.org/195283003/diff/60001/chrome/browser/ui/views/... > > chrome/browser/ui/views/screen_capture_notification_ui_views.cc:224: > > grip_rect.set_y(bounds().height() / 2 - grip_rect.height() / 2); > > nit: do grip_rect.set_y(bounds().height() - grip_rect.height()) / 2); as > below. > > > > > https://codereview.chromium.org/195283003/diff/60001/chrome/browser/ui/views/... > > chrome/browser/ui/views/screen_capture_notification_ui_views.cc:230: > > stop_button_rect.set_x(bounds().width() - stop_button_rect.width() - > > nit: Could you use a BoxLayout for this view? Otherwise, it seems like it'd be > > simpler to layout |hide_link_| before |stop_button_|, like this: > > hide_link_rect.set_x(bounds().width() - hide_link_rect.width()); > > ... > > stop_button_rect.set_x(hide_link_rect.x() - stop_button_rect.width() - > > kHorizontalMargin); > > > > > https://codereview.chromium.org/195283003/diff/60001/chrome/browser/ui/views/... > > chrome/browser/ui/views/screen_capture_notification_ui_views.cc:238: gfx::Rect > > label_rect(label_->GetPreferredSize()); > > nit: |label_|'s preferred size is not used at all, why bother making this > call? > > > > > https://codereview.chromium.org/195283003/diff/60001/chrome/browser/ui/views/... > > chrome/browser/ui/views/screen_capture_notification_ui_views.cc:247: > > stop_button_rect.width() + kHorizontalMargin + hide_link_rect.width(), > > nit: consider hide_link_rect.right() - stop_button_rect.x() > > > > > https://codereview.chromium.org/195283003/diff/60001/chrome/browser/ui/views/... > > chrome/browser/ui/views/screen_capture_notification_ui_views.cc:304: > > views::Link* source, int event_flags) { > > nit: one parameter per line. Also note that chromeos is not affected.
https://codereview.chromium.org/195283003/diff/80001/chrome/app/generated_res... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/195283003/diff/80001/chrome/app/generated_res... chrome/app/generated_resources.grd:9624: + <message name="IDS_MEDIA_SCREEN_CAPTURE_NOTIFICATION_HIDE" desc="Text shown on the button that hides the notification bar."> I'm not sure how you're planning on having a string translated in time for a release that has already branched, has that been discussed? Is there another existing hide string that you can use? https://codereview.chromium.org/195283003/diff/80001/chrome/browser/ui/views/... File chrome/browser/ui/views/screen_capture_notification_ui_views.cc (right): https://codereview.chromium.org/195283003/diff/80001/chrome/browser/ui/views/... chrome/browser/ui/views/screen_capture_notification_ui_views.cc:45: : views::ClientView(widget, view) { I know this isn't directly related to the issue, but the whole bubble looks odd with a white rect background the size of the contents and a gray round-rect background outside of that. Can you fix the background color here like below to match the bubble border and the parent ScreenCaptureNotificationUIViews? set_background(views::Background::CreateSolidBackground(GetNativeTheme()-> GetSystemColor(ui::NativeTheme::kColorId_DialogBackground))); https://codereview.chromium.org/195283003/diff/80001/chrome/browser/ui/views/... chrome/browser/ui/views/screen_capture_notification_ui_views.cc:230: hide_link_rect.set_x(bounds().width() - hide_link_rect.width()); I think this link looks weird with no padding on the right. I posted a mockup on the bug with the Hide link on the left of the Close button, which I think looks better; consider that.
PTAL. Thanks! https://codereview.chromium.org/195283003/diff/80001/chrome/app/generated_res... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/195283003/diff/80001/chrome/app/generated_res... chrome/app/generated_resources.grd:9624: + <message name="IDS_MEDIA_SCREEN_CAPTURE_NOTIFICATION_HIDE" desc="Text shown on the button that hides the notification bar."> Good point. Reusing an existing string then. On 2014/03/12 18:32:25, msw wrote: > I'm not sure how you're planning on having a string translated in time for a > release that has already branched, has that been discussed? Is there another > existing hide string that you can use? https://codereview.chromium.org/195283003/diff/80001/chrome/browser/ui/views/... File chrome/browser/ui/views/screen_capture_notification_ui_views.cc (right): https://codereview.chromium.org/195283003/diff/80001/chrome/browser/ui/views/... chrome/browser/ui/views/screen_capture_notification_ui_views.cc:45: : views::ClientView(widget, view) { I'm not comfortable adding unrelated change to this CL. You may open a new issue so we can fix it later. On 2014/03/12 18:32:25, msw wrote: > I know this isn't directly related to the issue, but the whole bubble looks odd > with a white rect background the size of the contents and a gray round-rect > background outside of that. Can you fix the background color here like below to > match the bubble border and the parent ScreenCaptureNotificationUIViews? > set_background(views::Background::CreateSolidBackground(GetNativeTheme()-> > GetSystemColor(ui::NativeTheme::kColorId_DialogBackground))); https://codereview.chromium.org/195283003/diff/80001/chrome/browser/ui/views/... chrome/browser/ui/views/screen_capture_notification_ui_views.cc:230: hide_link_rect.set_x(bounds().width() - hide_link_rect.width()); The window padding is set through views::BubbleFrameView* frame = new views::BubbleFrameView( gfx::Insets(kPadding, kPaddingLeft, kPadding, kPadding)); So I don't thinkg extra padding is needed. On 2014/03/12 18:32:25, msw wrote: > I think this link looks weird with no padding on the right. I posted a mockup on > the bug with the Hide link on the left of the Close button, which I think looks > better; consider that.
shrug, reluctant lgtm with a nit... Posting a full picture of the UI change on Mac would be nice, as would having a more substantial CL description. Perhaps Ben ought to take a look. https://codereview.chromium.org/195283003/diff/80001/chrome/browser/ui/views/... File chrome/browser/ui/views/screen_capture_notification_ui_views.cc (right): https://codereview.chromium.org/195283003/diff/80001/chrome/browser/ui/views/... chrome/browser/ui/views/screen_capture_notification_ui_views.cc:45: : views::ClientView(widget, view) { On 2014/03/12 19:08:34, jiayl wrote: > I'm not comfortable adding unrelated change to this CL. You may open a new issue > so we can fix it later. > > On 2014/03/12 18:32:25, msw wrote: > > I know this isn't directly related to the issue, but the whole bubble looks > odd > > with a white rect background the size of the contents and a gray round-rect > > background outside of that. Can you fix the background color here like below > to > > match the bubble border and the parent ScreenCaptureNotificationUIViews? > > set_background(views::Background::CreateSolidBackground(GetNativeTheme()-> > > GetSystemColor(ui::NativeTheme::kColorId_DialogBackground))); > I filed http://crbug.com/351839 https://codereview.chromium.org/195283003/diff/100001/chrome/browser/ui/views... File chrome/browser/ui/views/screen_capture_notification_ui_views.cc (right): https://codereview.chromium.org/195283003/diff/100001/chrome/browser/ui/views... chrome/browser/ui/views/screen_capture_notification_ui_views.cc:150: l10n_util::GetStringUTF16(IDS_PASSWORDS_PAGE_VIEW_HIDE_BUTTON)); nit: Add a TODO here to fix this up soon. I hope the translation for hiding a password and hiding (minimizing) a window are close enough.
The CQ bit was checked by jiayl@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jiayl@chromium.org/195283003/120001
The CQ bit was unchecked by commit-bot@chromium.org
Failed to apply patch for
chrome/browser/ui/views/screen_capture_notification_ui_views.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
patching file chrome/browser/ui/views/screen_capture_notification_ui_views.cc
Hunk #1 FAILED at 17.
1 out of 9 hunks FAILED -- saving rejects to file
chrome/browser/ui/views/screen_capture_notification_ui_views.cc.rej
Patch: chrome/browser/ui/views/screen_capture_notification_ui_views.cc
Index: chrome/browser/ui/views/screen_capture_notification_ui_views.cc
diff --git a/chrome/browser/ui/views/screen_capture_notification_ui_views.cc
b/chrome/browser/ui/views/screen_capture_notification_ui_views.cc
index
12371bc8a63c477ae78eab7662cff0411bb32326..c23358fa75b2a81c15d1eed3b137594ea383ced8
100644
--- a/chrome/browser/ui/views/screen_capture_notification_ui_views.cc
+++ b/chrome/browser/ui/views/screen_capture_notification_ui_views.cc
@@ -17,6 +17,8 @@
#include "ui/views/bubble/bubble_frame_view.h"
#include "ui/views/controls/button/blue_button.h"
#include "ui/views/controls/image_view.h"
+#include "ui/views/controls/link.h"
+#include "ui/views/controls/link_listener.h"
#include "ui/views/corewm/shadow_types.h"
#include "ui/views/layout/box_layout.h"
#include "ui/views/view.h"
@@ -71,7 +73,8 @@ class NotificationBarClientView : public views::ClientView {
class ScreenCaptureNotificationUIViews
: public ScreenCaptureNotificationUI,
public views::WidgetDelegateView,
- public views::ButtonListener {
+ public views::ButtonListener,
+ public views::LinkListener {
public:
explicit ScreenCaptureNotificationUIViews(const base::string16& text);
virtual ~ScreenCaptureNotificationUIViews();
@@ -92,11 +95,15 @@ class ScreenCaptureNotificationUIViews
virtual base::string16 GetWindowTitle() const OVERRIDE;
virtual bool ShouldShowWindowTitle() const OVERRIDE;
virtual bool ShouldShowCloseButton() const OVERRIDE;
+ virtual bool CanActivate() const OVERRIDE;
// views::ButtonListener interface.
virtual void ButtonPressed(views::Button* sender,
const ui::Event& event) OVERRIDE;
+ // views::LinkListener interface.
+ virtual void LinkClicked(views::Link* source, int event_flags) OVERRIDE;
+
private:
// Helper to call |stop_callback_|.
void NotifyStopped();
@@ -107,6 +114,7 @@ class ScreenCaptureNotificationUIViews
views::ImageView* gripper_;
views::Label* label_;
views::BlueButton* stop_button_;
+ views::Link* hide_link_;
DISALLOW_COPY_AND_ASSIGN(ScreenCaptureNotificationUIViews);
};
@@ -117,7 +125,8 @@
ScreenCaptureNotificationUIViews::ScreenCaptureNotificationUIViews(
client_view_(NULL),
gripper_(NULL),
label_(NULL),
- stop_button_(NULL) {
+ stop_button_(NULL),
+ hide_link_(NULL) {
set_owned_by_client();
set_background(views::Background::CreateSolidBackground(GetNativeTheme()->
@@ -136,6 +145,13 @@
ScreenCaptureNotificationUIViews::ScreenCaptureNotificationUIViews(
l10n_util::GetStringUTF16(IDS_MEDIA_SCREEN_CAPTURE_NOTIFICATION_STOP);
stop_button_ = new views::BlueButton(this, stop_text);
AddChildView(stop_button_);
+
+ // TODO(jiayl): IDS_PASSWORDS_PAGE_VIEW_HIDE_BUTTON is used for the need to
+ // merge to M34. Change it to a new IDS_ after the merge.
+ hide_link_ = new views::Link(
+ l10n_util::GetStringUTF16(IDS_PASSWORDS_PAGE_VIEW_HIDE_BUTTON));
+ hide_link_->set_listener(this);
+ AddChildView(hide_link_);
}
ScreenCaptureNotificationUIViews::~ScreenCaptureNotificationUIViews() {
@@ -160,7 +176,8 @@ void ScreenCaptureNotificationUIViews::OnStarted(
params.remove_standard_frame = true;
params.keep_on_top = true;
params.top_level = true;
- params.can_activate = false;
+ // Make sure can_activate is true so the window icon will show in the
taskbar.
+ params.can_activate = true;
#if defined(USE_ASH)
// TODO(sergeyu): The notification bar must be shown on the monitor that's
@@ -192,31 +209,45 @@ void ScreenCaptureNotificationUIViews::OnStarted(
gfx::Size ScreenCaptureNotificationUIViews::GetPreferredSize() {
gfx::Size grip_size = gripper_->GetPreferredSize();
- gfx::Size label_size = child_at(1)->GetPreferredSize();
- gfx::Size button_size = child_at(2)->GetPreferredSize();
- int width = kHorizontalMargin * 2 + grip_size.width() + label_size.width() +
- button_size.width();
+ gfx::Size label_size = label_->GetPreferredSize();
+ gfx::Size stop_button_size = stop_button_->GetPreferredSize();
+ gfx::Size hide_link_size = hide_link_->GetPreferredSize();
+ int width = kHorizontalMargin * 3 + grip_size.width() + label_size.width() +
+ stop_button_size.width() + hide_link_size.width();
width = std::max(width, kMinimumWidth);
width = std::min(width, kMaximumWidth);
- return gfx::Size(width, std::max(label_size.height(), button_size.height()));
+ return gfx::Size(width, std::max(label_size.height(),
+ std::max(hide_link_size.height(),
+ stop_button_size.height())));
}
void ScreenCaptureNotificationUIViews::Layout() {
gfx::Rect grip_rect(gripper_->GetPreferredSize());
- grip_rect.set_y(bounds().height() / 2 - grip_rect.height() / 2);
+ grip_rect.set_y((bounds().height() - grip_rect.height()) / 2);
gripper_->SetBoundsRect(grip_rect);
- gfx::Rect button_rect(stop_button_->GetPreferredSize());
- button_rect.set_x(bounds().width() - button_rect.width());
- stop_button_->SetBoundsRect(button_rect);
+ gfx::Rect stop_button_rect(stop_button_->GetPreferredSize());
+ gfx::Rect hide_link_rect(hide_link_->GetPreferredSize());
+
+ hide_link_rect.set_x(bounds().width() - hide_link_rect.width());
+ hide_link_rect.set_y((bounds().height() - hide_link_rect.height()) / 2);
+ hide_link_->SetBoundsRect(hide_link_rect);
+
+ stop_button_rect.set_x(
+ hide_link_rect.x() - kHorizontalMargin - stop_button_rect.width());
+ stop_button_->SetBoundsRect(stop_button_rect);
gfx::Rect label_rect;
label_rect.set_x(grip_rect.right() + kHorizontalMargin);
- label_rect.set_width(button_rect.x() - kHorizontalMargin - label_rect.x());
+ label_rect.set_width(
+ stop_button_rect.x() - kHorizontalMargin - label_rect.x());
label_rect.set_height(bounds().height());
label_->SetBoundsRect(label_rect);
- client_view_->SetClientRect(button_rect);
+ client_view_->SetClientRect(gfx::Rect(
+ stop_button_rect.x(), stop_button_rect.y(),
+ stop_button_rect.width() + kHorizontalMargin + hide_link_rect.width(),
+ std::max(stop_button_rect.height(), hide_link_rect.height())));
}
void ScreenCaptureNotificationUIViews::DeleteDelegate() {
@@ -260,11 +291,22 @@ bool
ScreenCaptureNotificationUIViews::ShouldShowCloseButton() const {
return false;
}
+bool ScreenCaptureNotificationUIViews::CanActivate() const {
+ // If we do not override this method, the window sometimes does not properly
+ // restore to its normal size on Windows.
+ return false;
+}
+
void ScreenCaptureNotificationUIViews::ButtonPressed(views::Button* sender,
const ui::Event& event) {
NotifyStopped();
}
+void ScreenCaptureNotificationUIViews::LinkClicked(views::Link* source,
+ int event_flags) {
+ GetWidget()->Minimize();
+}
+
void ScreenCaptureNotificationUIViews::NotifyStopped() {
if (!stop_callback_.is_null()) {
base::Closure callback = stop_callback_;
The CQ bit was unchecked by jiayl@chromium.org
The CQ bit was checked by jiayl@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jiayl@chromium.org/195283003/140001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.chromium on mac_chromium_compile_dbg
The CQ bit was checked by jiayl@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jiayl@chromium.org/195283003/140001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.chromium on mac_chromium_rel
The CQ bit was checked by jiayl@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jiayl@chromium.org/195283003/140001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.chromium on mac_chromium_rel
The CQ bit was checked by jiayl@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jiayl@chromium.org/195283003/140001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.chromium on linux_chromium_chromeos_clang_dbg
The CQ bit was checked by jiayl@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jiayl@chromium.org/195283003/140001
Message was sent while issue was closed.
Change committed as 256851 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
