Skip to content

Commit

Permalink
Revert "capture_mode: Capture bar does not intersect autoclick."
Browse files Browse the repository at this point in the history
This reverts commit 8cd3185.

Reason for revert: suspect causing browser_tests failure on linux-chromeos-rel

Failing tests:
AccessibilityPolicyTest.AutoclickEnabled
All/GestureNavigationScreenTest.ScreenSkippedWithAutoclickEnabled/1
AutoclickE2ETest.HighlightsRootWebAreaIfNotScrollable
AutoclickE2ETest.RemovesAndAddsAutoclick
ChromeOSInfoPrivateTest.TestGetAndSet

First test failure: https://ci.chromium.org/p/chromium/builders/ci/linux-chromeos-rel/45848

Sample log:
BrowserTestBase received signal: Segmentation fault. Backtrace:
...
#3 0x7fa7483ba4c0 (/lib/x86_64-linux-gnu/libc-2.23.so+0x354bf)
#4 0x5598f0b6ecce ash::CaptureModeController::IsActive()
#5 0x5598f0d32b28 ash::CollisionDetectionUtils::AvoidObstacles()
#6 0x5598f0d32296 ash::CollisionDetectionUtils::GetRestingPosition()
#7 0x5598f0c73a68 ash::AutoclickMenuBubbleController::SetPosition()
#8 0x5598f0d8c661 ash::WorkspaceLayoutManager::NotifySystemUiAreaChanged()

Original change's description:
> capture_mode: Capture bar does not intersect autoclick.
>
> Adds capture mode on the collision course for bubbles like PIP and
> autoclick, and triggers repositioning of the autoclick on enter or exit
> capture mode.
>
> Fixed: 1149758
> Test: manual
> Change-Id: I800b2b265374627718cd944d39d76dc4e94be7fb
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2597219
> Reviewed-by: Ahmed Fakhry <afakhry@chromium.org>
> Commit-Queue: Sammie Quon <sammiequon@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#859045}

Change-Id: I2ee7dbd24ed3006a494b59b8dd93cbef86b0353a
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2730374
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Reviewed-by: Takashi Sakamoto <tasak@google.com>
Commit-Queue: Takashi Sakamoto <tasak@google.com>
Cr-Commit-Position: refs/heads/master@{#859248}
  • Loading branch information
tasak authored and Chromium LUCI CQ committed Mar 3, 2021
1 parent 96c47a9 commit 584d76f
Show file tree
Hide file tree
Showing 9 changed files with 17 additions and 62 deletions.
6 changes: 0 additions & 6 deletions ash/capture_mode/capture_mode_controller.cc
Original file line number Diff line number Diff line change
Expand Up @@ -330,10 +330,6 @@ CaptureModeController* CaptureModeController::Get() {
return g_instance;
}

bool CaptureModeController::IsActive() const {
return capture_mode_session_ && !capture_mode_session_->is_shutting_down();
}

void CaptureModeController::SetSource(CaptureModeSource source) {
if (source == source_)
return;
Expand Down Expand Up @@ -399,13 +395,11 @@ void CaptureModeController::Start(CaptureModeEntryType entry_type) {
delegate_->OnSessionStateChanged(/*started=*/true);

capture_mode_session_ = std::make_unique<CaptureModeSession>(this);
capture_mode_session_->Initialize();
}

void CaptureModeController::Stop() {
DCHECK(IsActive());
capture_mode_session_->ReportSessionHistograms();
capture_mode_session_->Shutdown();
capture_mode_session_.reset();

delegate_->OnSessionStateChanged(/*started=*/false);
Expand Down
2 changes: 1 addition & 1 deletion ash/capture_mode/capture_mode_controller.h
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ class ASH_EXPORT CaptureModeController
bool is_recording_in_progress() const { return is_recording_in_progress_; }

// Returns true if a capture mode session is currently active.
bool IsActive() const;
bool IsActive() const { return !!capture_mode_session_; }

// Sets the capture source/type, which will be applied to an ongoing capture
// session (if any), or to a future capture session when Start() is called.
Expand Down
16 changes: 2 additions & 14 deletions ash/capture_mode/capture_mode_session.cc
Original file line number Diff line number Diff line change
Expand Up @@ -314,10 +314,6 @@ int GetMessageIdForCaptureSource(CaptureModeSource source,
}
}

void UpdateAutoclickMenuBoundsIfNeeded() {
Shell::Get()->accessibility_controller()->UpdateAutoclickMenuBoundsIfNeeded();
}

} // namespace

// -----------------------------------------------------------------------------
Expand Down Expand Up @@ -496,11 +492,7 @@ CaptureModeSession::CaptureModeSession(CaptureModeController* controller)
current_root_(GetPreferredRootWindow()),
magnifier_glass_(kMagnifierParams),
cursor_setter_(std::make_unique<CursorSetter>()),
focus_cycler_(std::make_unique<CaptureModeSessionFocusCycler>(this)) {}

CaptureModeSession::~CaptureModeSession() = default;

void CaptureModeSession::Initialize() {
focus_cycler_(std::make_unique<CaptureModeSessionFocusCycler>(this)) {
// A context menu may have input capture when entering a session. Remove
// capture from it, otherwise subsequent mouse events will cause it to close,
// and then we won't be able to take a screenshot of the menu. Store it so we
Expand Down Expand Up @@ -571,12 +563,9 @@ void CaptureModeSession::Initialize() {
controller_->type() == CaptureModeType::kImage
? IDS_ASH_SCREEN_CAPTURE_TYPE_SCREENSHOT
: IDS_ASH_SCREEN_CAPTURE_TYPE_SCREEN_RECORDING)));
UpdateAutoclickMenuBoundsIfNeeded();
}

void CaptureModeSession::Shutdown() {
is_shutting_down_ = true;

CaptureModeSession::~CaptureModeSession() {
aura::Env::GetInstance()->RemovePreTargetHandler(this);
display::Screen::GetScreen()->RemoveObserver(this);
current_root_->RemoveObserver(this);
Expand Down Expand Up @@ -606,7 +595,6 @@ void CaptureModeSession::Shutdown() {
capture_mode_util::TriggerAccessibilityAlert(
IDS_ASH_SCREEN_CAPTURE_ALERT_CLOSE);
}
UpdateAutoclickMenuBoundsIfNeeded();
}

aura::Window* CaptureModeSession::GetSelectedWindow() const {
Expand Down
15 changes: 0 additions & 15 deletions ash/capture_mode/capture_mode_session.h
Original file line number Diff line number Diff line change
Expand Up @@ -65,23 +65,11 @@ class ASH_EXPORT CaptureModeSession : public ui::LayerOwner,
static constexpr int kCaptureButtonDistanceFromRegionDp = 24;

aura::Window* current_root() const { return current_root_; }
views::Widget* capture_mode_bar_widget() {
return capture_mode_bar_widget_.get();
}
bool is_selecting_region() const { return is_selecting_region_; }
bool is_drag_in_progress() const { return is_drag_in_progress_; }
void set_a11y_alert_on_session_exit(bool value) {
a11y_alert_on_session_exit_ = value;
}
bool is_shutting_down() const { return is_shutting_down_; }

// Initializes the capture mode session. This should be called right after the
// object is created.
void Initialize();

// Shuts down the capture mode session. This should be called right before the
// object is destroyed.
void Shutdown();

// Gets the current window selected for |kWindow| capture source. Returns
// nullptr if no window is available for selection.
Expand Down Expand Up @@ -345,9 +333,6 @@ class ASH_EXPORT CaptureModeSession : public ui::LayerOwner,
// False only when we end the session to start recording.
bool a11y_alert_on_session_exit_ = true;

// True once Shutdown() is called.
bool is_shutting_down_ = false;

// The object which handles tab focus while in a capture session.
std::unique_ptr<CaptureModeSessionFocusCycler> focus_cycler_;

Expand Down
9 changes: 7 additions & 2 deletions ash/capture_mode/capture_mode_unittests.cc
Original file line number Diff line number Diff line change
Expand Up @@ -199,6 +199,10 @@ class CaptureModeSessionTestApi {
return session_->capture_mode_bar_view_;
}

views::Widget* capture_mode_bar_widget() const {
return session_->capture_mode_bar_widget_.get();
}

CaptureModeSettingsView* capture_mode_settings_view() const {
return session_->capture_mode_settings_view_;
}
Expand Down Expand Up @@ -261,7 +265,7 @@ class CaptureModeTest : public AshTestBase {
views::Widget* GetCaptureModeBarWidget() const {
auto* session = CaptureModeController::Get()->capture_mode_session();
DCHECK(session);
return session->capture_mode_bar_widget();
return CaptureModeSessionTestApi(session).capture_mode_bar_widget();
}

views::Widget* GetCaptureModeLabelWidget() const {
Expand Down Expand Up @@ -507,7 +511,8 @@ TEST_F(CaptureModeTest, StartStop) {

// Closing the session should close the native window of capture mode bar
// immediately.
auto* bar_window = GetCaptureModeBarWidget()->GetNativeWindow();
CaptureModeSessionTestApi test_api(controller->capture_mode_session());
auto* bar_window = test_api.capture_mode_bar_widget()->GetNativeWindow();
aura::WindowTracker tracker({bar_window});
controller->Stop();
EXPECT_TRUE(tracker.windows().empty());
Expand Down
9 changes: 4 additions & 5 deletions ash/wm/always_on_top_controller.cc
Original file line number Diff line number Diff line change
Expand Up @@ -42,11 +42,6 @@ AlwaysOnTopController::~AlwaysOnTopController() {
DCHECK(!pip_container_);
}

// static
void AlwaysOnTopController::SetDisallowReparent(aura::Window* window) {
window->SetProperty(kDisallowReparentKey, true);
}

aura::Window* AlwaysOnTopController::GetContainer(aura::Window* window) const {
DCHECK(always_on_top_container_);
DCHECK(pip_container_);
Expand Down Expand Up @@ -83,6 +78,10 @@ void AlwaysOnTopController::SetLayoutManagerForTest(
always_on_top_container_->SetLayoutManager(layout_manager.release());
}

void AlwaysOnTopController::SetDisallowReparent(aura::Window* window) {
window->SetProperty(kDisallowReparentKey, true);
}

void AlwaysOnTopController::AddWindow(aura::Window* window) {
window->AddObserver(this);
WindowState::Get(window)->AddObserver(this);
Expand Down
4 changes: 2 additions & 2 deletions ash/wm/always_on_top_controller.h
Original file line number Diff line number Diff line change
Expand Up @@ -27,14 +27,14 @@ class ASH_EXPORT AlwaysOnTopController : public aura::WindowObserver,
aura::Window* pip_container);
~AlwaysOnTopController() override;

static void SetDisallowReparent(aura::Window* window);

// Gets container for given |window| based on its "AlwaysOnTop" property.
aura::Window* GetContainer(aura::Window* window) const;

void SetLayoutManagerForTest(
std::unique_ptr<WorkspaceLayoutManager> layout_manager);

static void SetDisallowReparent(aura::Window* window);

private:
void AddWindow(aura::Window* window);
void RemoveWindow(aura::Window* window);
Expand Down
16 changes: 0 additions & 16 deletions ash/wm/collision_detection/collision_detection_utils.cc
Original file line number Diff line number Diff line change
Expand Up @@ -4,10 +4,7 @@

#include "ash/wm/collision_detection/collision_detection_utils.h"

#include "ash/capture_mode/capture_mode_controller.h"
#include "ash/capture_mode/capture_mode_session.h"
#include "ash/keyboard/ui/keyboard_ui_controller.h"
#include "ash/public/cpp/ash_features.h"
#include "ash/public/cpp/shelf_types.h"
#include "ash/public/cpp/shell_window_ids.h"
#include "ash/shelf/shelf.h"
Expand Down Expand Up @@ -155,19 +152,6 @@ std::vector<gfx::Rect> CollectCollisionRects(
/*parent=*/root_window));
}

// Check the capture bar if capture mode is active.
if (features::IsCaptureModeEnabled()) {
auto* capture_mode_controller = CaptureModeController::Get();
if (capture_mode_controller->IsActive()) {
aura::Window* capture_bar_window =
capture_mode_controller->capture_mode_session()
->capture_mode_bar_widget()
->GetNativeWindow();
rects.push_back(ComputeCollisionRectFromBounds(
capture_bar_window->GetTargetBounds(), capture_bar_window->parent()));
}
}

return rects;
}

Expand Down
2 changes: 1 addition & 1 deletion ash/wm/workspace/workspace_layout_manager.h
Original file line number Diff line number Diff line change
Expand Up @@ -153,7 +153,7 @@ class ASH_EXPORT WorkspaceLayoutManager : public aura::LayoutManager,

// Notifies windows about a change in a system ui area. This could be
// the keyboard or any window in the SettingsBubbleContainer or
// |accessibility_bubble_container_|. Windows will only be notified about
// accessibility_bubble_container_. Windows will only be notified about
// changes to system ui areas on the display they are on.
void NotifySystemUiAreaChanged();

Expand Down

0 comments on commit 584d76f

Please sign in to comment.