Commit 7683e344 authored by Steven Bennetts's avatar Steven Bennetts Committed by Commit Bot

Shell: Move DisplayManager initialization to a separate method

This CL:
* Moves a bunch of related DisplayManager and DisplayConfigurator
  code to a separate InitializeDisplayManager method to help improve
  managability of Shell::Init.
* Renames ShutdownObserver -> DisplayShutdownObserver.
* Removes unnecessary DBusThreadManager dependencies from shell.cc.
* Moves observer responsibility from Shell to ProjectingObserver

Bug: 678949
Change-Id: I07159d0562bc032f03c653beb927ab8aaf053bbc
Reviewed-on: https://chromium-review.googlesource.com/833500Reviewed-by: default avatarScott Violet <sky@chromium.org>
Reviewed-by: default avatarJames Cook <jamescook@chromium.org>
Commit-Queue: Steven Bennetts <stevenjb@chromium.org>
Cr-Commit-Position: refs/heads/master@{#526741}
parent abf98057
......@@ -108,6 +108,8 @@ component("ash") {
"display/display_error_observer_chromeos.h",
"display/display_move_window_util.cc",
"display/display_move_window_util.h",
"display/display_shutdown_observer.cc",
"display/display_shutdown_observer.h",
"display/display_synchronizer.cc",
"display/display_synchronizer.h",
"display/display_util.cc",
......@@ -139,8 +141,6 @@ component("ash") {
"display/screen_position_controller.h",
"display/shared_display_edge_indicator.cc",
"display/shared_display_edge_indicator.h",
"display/shutdown_observer_chromeos.cc",
"display/shutdown_observer_chromeos.h",
"display/touch_calibrator_controller.cc",
"display/touch_calibrator_controller.h",
"display/touch_calibrator_view.cc",
......
......@@ -2,22 +2,22 @@
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
#include "ash/display/shutdown_observer_chromeos.h"
#include "ash/display/display_shutdown_observer.h"
#include "ui/display/manager/chromeos/display_configurator.h"
namespace ash {
ShutdownObserver::ShutdownObserver(
DisplayShutdownObserver::DisplayShutdownObserver(
display::DisplayConfigurator* display_configurator)
: display_configurator_(display_configurator),
scoped_session_observer_(this) {}
ShutdownObserver::~ShutdownObserver() = default;
DisplayShutdownObserver::~DisplayShutdownObserver() = default;
void ShutdownObserver::OnChromeTerminating() {
void DisplayShutdownObserver::OnChromeTerminating() {
// Stop handling display configuration events once the shutdown
// process starts. crbug.com/177014.
// process starts. http://crbug.com/177014.
display_configurator_->PrepareForExit();
}
......
......@@ -2,8 +2,8 @@
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
#ifndef ASH_DISPLAY_SHUTDOWN_OBSERVER_CHROMEOS_H_
#define ASH_DISPLAY_SHUTDOWN_OBSERVER_CHROMEOS_H_
#ifndef ASH_DISPLAY_DISPLAY_SHUTDOWN_OBSERVER_H_
#define ASH_DISPLAY_DISPLAY_SHUTDOWN_OBSERVER_H_
#include "ash/session/session_observer.h"
#include "base/macros.h"
......@@ -16,10 +16,11 @@ namespace ash {
// Adds self as SessionObserver and listens for OnChromeTerminating() on
// behalf of |display_configurator_|.
class ShutdownObserver : public SessionObserver {
class DisplayShutdownObserver : public SessionObserver {
public:
explicit ShutdownObserver(display::DisplayConfigurator* display_configurator);
~ShutdownObserver() override;
explicit DisplayShutdownObserver(
display::DisplayConfigurator* display_configurator);
~DisplayShutdownObserver() override;
private:
// SessionObserver:
......@@ -28,9 +29,9 @@ class ShutdownObserver : public SessionObserver {
display::DisplayConfigurator* const display_configurator_;
ScopedSessionObserver scoped_session_observer_;
DISALLOW_COPY_AND_ASSIGN(ShutdownObserver);
DISALLOW_COPY_AND_ASSIGN(DisplayShutdownObserver);
};
} // namespace ash
#endif // ASH_DISPLAY_SHUTDOWN_OBSERVER_CHROMEOS_H_
#endif // ASH_DISPLAY_DISPLAY_SHUTDOWN_OBSERVER_H_
......@@ -4,22 +4,29 @@
#include "ash/display/projecting_observer_chromeos.h"
#include "ash/shell.h"
#include "base/logging.h"
#include "chromeos/dbus/dbus_thread_manager.h"
#include "chromeos/dbus/power_manager_client.h"
#include "ui/display/types/display_snapshot.h"
namespace ash {
ProjectingObserver::ProjectingObserver(
chromeos::PowerManagerClient* power_manager_client)
: has_internal_output_(false),
output_count_(0),
casting_session_count_(0),
power_manager_client_(power_manager_client) {
DCHECK(power_manager_client);
display::DisplayConfigurator* display_configurator)
: display_configurator_(display_configurator) {
if (Shell::HasInstance())
Shell::Get()->AddShellObserver(this);
if (display_configurator_)
display_configurator_->AddObserver(this);
}
ProjectingObserver::~ProjectingObserver() = default;
ProjectingObserver::~ProjectingObserver() {
if (Shell::HasInstance())
Shell::Get()->RemoveShellObserver(this);
if (display_configurator_)
display_configurator_->RemoveObserver(this);
}
void ProjectingObserver::OnDisplayModeChanged(
const display::DisplayConfigurator::DisplayStateList& display_states) {
......@@ -56,7 +63,12 @@ void ProjectingObserver::SetIsProjecting() {
bool projecting =
has_internal_output_ && (output_count_ + casting_session_count_ > 1);
power_manager_client_->SetIsProjecting(projecting);
chromeos::PowerManagerClient* power_manager_client =
power_manager_client_for_test_
? power_manager_client_for_test_
: chromeos::DBusThreadManager::Get()->GetPowerManagerClient();
if (power_manager_client)
power_manager_client->SetIsProjecting(projecting);
}
} // namespace ash
......@@ -20,9 +20,9 @@ class ASH_EXPORT ProjectingObserver
: public display::DisplayConfigurator::Observer,
public ShellObserver {
public:
// |power_manager_client| must outlive this object.
// |display_configurator| must outlive this instance. May be null in tests.
explicit ProjectingObserver(
chromeos::PowerManagerClient* power_manager_client);
display::DisplayConfigurator* display_configurator);
~ProjectingObserver() override;
// DisplayConfigurator::Observer implementation:
......@@ -33,21 +33,30 @@ class ASH_EXPORT ProjectingObserver
void OnCastingSessionStartedOrStopped(bool started) override;
private:
friend class ProjectingObserverTest;
void set_power_manager_client_for_test(
chromeos::PowerManagerClient* power_manager_client) {
power_manager_client_for_test_ = power_manager_client;
}
// Sends the current projecting state to power manager.
void SetIsProjecting();
display::DisplayConfigurator* display_configurator_; // Unowned
// True if at least one output is internal. This value is updated when
// |OnDisplayModeChanged| is called.
bool has_internal_output_;
bool has_internal_output_ = false;
// Keeps track of the number of connected outputs.
int output_count_;
int output_count_ = 0;
// Number of outstanding casting sessions.
int casting_session_count_;
int casting_session_count_ = 0;
// Weak pointer to the DBusClient PowerManagerClient;
chromeos::PowerManagerClient* power_manager_client_;
// Weak pointer to the DBusClient PowerManagerClient for testing;
chromeos::PowerManagerClient* power_manager_client_for_test_ = nullptr;
DISALLOW_COPY_AND_ASSIGN(ProjectingObserver);
};
......
......@@ -12,6 +12,7 @@
#include "ui/display/manager/fake_display_snapshot.h"
namespace ash {
namespace {
std::unique_ptr<display::DisplaySnapshot> CreateInternalSnapshot() {
......@@ -30,33 +31,36 @@ std::unique_ptr<display::DisplaySnapshot> CreateVGASnapshot() {
.Build();
}
display::DisplayConfigurator::DisplayStateList GetPointers(
const std::vector<std::unique_ptr<display::DisplaySnapshot>>& displays) {
display::DisplayConfigurator::DisplayStateList result;
for (const auto& display : displays)
result.push_back(display.get());
return result;
}
} // namespace
class ProjectingObserverTest : public testing::Test {
public:
ProjectingObserverTest() : observer_(&fake_power_client_) {}
ProjectingObserverTest() {
observer_ = std::make_unique<ProjectingObserver>(nullptr);
observer_->set_power_manager_client_for_test(&fake_power_client_);
}
~ProjectingObserverTest() override = default;
protected:
chromeos::FakePowerManagerClient fake_power_client_;
ProjectingObserver observer_;
std::unique_ptr<ProjectingObserver> observer_;
private:
DISALLOW_COPY_AND_ASSIGN(ProjectingObserverTest);
};
display::DisplayConfigurator::DisplayStateList GetPointers(
const std::vector<std::unique_ptr<display::DisplaySnapshot>>& displays) {
display::DisplayConfigurator::DisplayStateList result;
for (const auto& display : displays)
result.push_back(display.get());
return result;
}
} // namespace
TEST_F(ProjectingObserverTest, CheckNoDisplay) {
std::vector<std::unique_ptr<display::DisplaySnapshot>> displays;
observer_.OnDisplayModeChanged(GetPointers(displays));
observer_->OnDisplayModeChanged(GetPointers(displays));
EXPECT_EQ(1, fake_power_client_.num_set_is_projecting_calls());
EXPECT_FALSE(fake_power_client_.is_projecting());
......@@ -65,7 +69,7 @@ TEST_F(ProjectingObserverTest, CheckNoDisplay) {
TEST_F(ProjectingObserverTest, CheckWithoutInternalDisplay) {
std::vector<std::unique_ptr<display::DisplaySnapshot>> displays;
displays.push_back(CreateVGASnapshot());
observer_.OnDisplayModeChanged(GetPointers(displays));
observer_->OnDisplayModeChanged(GetPointers(displays));
EXPECT_EQ(1, fake_power_client_.num_set_is_projecting_calls());
EXPECT_FALSE(fake_power_client_.is_projecting());
......@@ -74,7 +78,7 @@ TEST_F(ProjectingObserverTest, CheckWithoutInternalDisplay) {
TEST_F(ProjectingObserverTest, CheckWithInternalDisplay) {
std::vector<std::unique_ptr<display::DisplaySnapshot>> displays;
displays.push_back(CreateInternalSnapshot());
observer_.OnDisplayModeChanged(GetPointers(displays));
observer_->OnDisplayModeChanged(GetPointers(displays));
EXPECT_EQ(1, fake_power_client_.num_set_is_projecting_calls());
EXPECT_FALSE(fake_power_client_.is_projecting());
......@@ -84,7 +88,7 @@ TEST_F(ProjectingObserverTest, CheckWithTwoVGADisplays) {
std::vector<std::unique_ptr<display::DisplaySnapshot>> displays;
displays.push_back(CreateVGASnapshot());
displays.push_back(CreateVGASnapshot());
observer_.OnDisplayModeChanged(GetPointers(displays));
observer_->OnDisplayModeChanged(GetPointers(displays));
EXPECT_EQ(1, fake_power_client_.num_set_is_projecting_calls());
// We need at least 1 internal display to set projecting to on.
......@@ -95,7 +99,7 @@ TEST_F(ProjectingObserverTest, CheckWithInternalAndVGADisplays) {
std::vector<std::unique_ptr<display::DisplaySnapshot>> displays;
displays.push_back(CreateInternalSnapshot());
displays.push_back(CreateVGASnapshot());
observer_.OnDisplayModeChanged(GetPointers(displays));
observer_->OnDisplayModeChanged(GetPointers(displays));
EXPECT_EQ(1, fake_power_client_.num_set_is_projecting_calls());
EXPECT_TRUE(fake_power_client_.is_projecting());
......@@ -104,9 +108,9 @@ TEST_F(ProjectingObserverTest, CheckWithInternalAndVGADisplays) {
TEST_F(ProjectingObserverTest, CheckWithVGADisplayAndOneCastingSession) {
std::vector<std::unique_ptr<display::DisplaySnapshot>> displays;
displays.push_back(CreateVGASnapshot());
observer_.OnDisplayModeChanged(GetPointers(displays));
observer_->OnDisplayModeChanged(GetPointers(displays));
observer_.OnCastingSessionStartedOrStopped(true);
observer_->OnCastingSessionStartedOrStopped(true);
EXPECT_EQ(2, fake_power_client_.num_set_is_projecting_calls());
// Need at least one internal display to set projecting state to |true|.
......@@ -116,9 +120,9 @@ TEST_F(ProjectingObserverTest, CheckWithVGADisplayAndOneCastingSession) {
TEST_F(ProjectingObserverTest, CheckWithInternalDisplayAndOneCastingSession) {
std::vector<std::unique_ptr<display::DisplaySnapshot>> displays;
displays.push_back(CreateInternalSnapshot());
observer_.OnDisplayModeChanged(GetPointers(displays));
observer_->OnDisplayModeChanged(GetPointers(displays));
observer_.OnCastingSessionStartedOrStopped(true);
observer_->OnCastingSessionStartedOrStopped(true);
EXPECT_EQ(2, fake_power_client_.num_set_is_projecting_calls());
EXPECT_TRUE(fake_power_client_.is_projecting());
......@@ -127,15 +131,15 @@ TEST_F(ProjectingObserverTest, CheckWithInternalDisplayAndOneCastingSession) {
TEST_F(ProjectingObserverTest, CheckProjectingAfterClosingACastingSession) {
std::vector<std::unique_ptr<display::DisplaySnapshot>> displays;
displays.push_back(CreateInternalSnapshot());
observer_.OnDisplayModeChanged(GetPointers(displays));
observer_->OnDisplayModeChanged(GetPointers(displays));
observer_.OnCastingSessionStartedOrStopped(true);
observer_.OnCastingSessionStartedOrStopped(true);
observer_->OnCastingSessionStartedOrStopped(true);
observer_->OnCastingSessionStartedOrStopped(true);
EXPECT_EQ(3, fake_power_client_.num_set_is_projecting_calls());
EXPECT_TRUE(fake_power_client_.is_projecting());
observer_.OnCastingSessionStartedOrStopped(false);
observer_->OnCastingSessionStartedOrStopped(false);
EXPECT_EQ(4, fake_power_client_.num_set_is_projecting_calls());
EXPECT_TRUE(fake_power_client_.is_projecting());
......@@ -145,10 +149,10 @@ TEST_F(ProjectingObserverTest,
CheckStopProjectingAfterClosingAllCastingSessions) {
std::vector<std::unique_ptr<display::DisplaySnapshot>> displays;
displays.push_back(CreateInternalSnapshot());
observer_.OnDisplayModeChanged(GetPointers(displays));
observer_->OnDisplayModeChanged(GetPointers(displays));
observer_.OnCastingSessionStartedOrStopped(true);
observer_.OnCastingSessionStartedOrStopped(false);
observer_->OnCastingSessionStartedOrStopped(true);
observer_->OnCastingSessionStartedOrStopped(false);
EXPECT_EQ(3, fake_power_client_.num_set_is_projecting_calls());
EXPECT_FALSE(fake_power_client_.is_projecting());
......@@ -159,11 +163,11 @@ TEST_F(ProjectingObserverTest,
std::vector<std::unique_ptr<display::DisplaySnapshot>> displays;
displays.push_back(CreateInternalSnapshot());
displays.push_back(CreateVGASnapshot());
observer_.OnDisplayModeChanged(GetPointers(displays));
observer_->OnDisplayModeChanged(GetPointers(displays));
// Remove VGA output.
displays.erase(displays.begin() + 1);
observer_.OnDisplayModeChanged(GetPointers(displays));
observer_->OnDisplayModeChanged(GetPointers(displays));
EXPECT_EQ(2, fake_power_client_.num_set_is_projecting_calls());
EXPECT_FALSE(fake_power_client_.is_projecting());
......
......@@ -25,6 +25,7 @@
#include "ash/display/display_color_manager_chromeos.h"
#include "ash/display/display_configuration_controller.h"
#include "ash/display/display_error_observer_chromeos.h"
#include "ash/display/display_shutdown_observer.h"
#include "ash/display/event_transformation_handler.h"
#include "ash/display/mouse_cursor_event_filter.h"
#include "ash/display/projecting_observer_chromeos.h"
......@@ -32,7 +33,6 @@
#include "ash/display/screen_ash.h"
#include "ash/display/screen_orientation_controller_chromeos.h"
#include "ash/display/screen_position_controller.h"
#include "ash/display/shutdown_observer_chromeos.h"
#include "ash/display/window_tree_host_manager.h"
#include "ash/drag_drop/drag_drop_controller.h"
#include "ash/first_run/first_run_helper_impl.h"
......@@ -141,7 +141,6 @@
#include "base/threading/sequenced_worker_pool.h"
#include "base/trace_event/trace_event.h"
#include "chromeos/chromeos_switches.h"
#include "chromeos/dbus/dbus_thread_manager.h"
#include "chromeos/system/devicemode.h"
#include "components/prefs/pref_registry_simple.h"
#include "components/prefs/pref_service.h"
......@@ -786,22 +785,17 @@ Shell::~Shell() {
screen_position_controller_.reset();
display_color_manager_.reset();
projecting_observer_.reset();
if (display_change_observer_)
display_configurator_->RemoveObserver(display_change_observer_.get());
if (display_error_observer_)
display_configurator_->RemoveObserver(display_error_observer_.get());
if (projecting_observer_) {
display_configurator_->RemoveObserver(projecting_observer_.get());
RemoveShellObserver(projecting_observer_.get());
}
display_change_observer_.reset();
shutdown_observer_.reset();
display_shutdown_observer_.reset();
PowerStatus::Shutdown();
// Ensure that DBusThreadManager outlives this Shell.
DCHECK(chromeos::DBusThreadManager::IsInitialized());
// Needs to happen right before |instance_| is reset.
shell_port_.reset();
session_controller_->RemoveObserver(this);
......@@ -881,65 +875,19 @@ void Shell::Init(ui::ContextFactory* context_factory,
base::WrapUnique(native_cursor_manager_));
}
// TODO(stevenjb): ChromeShellDelegate::PreInit currently handles
// DisplayPreference initialization, required for InitializeDisplayManager.
// Before we can move that code into ash/display where it belongs, we need to
// wait for |lcoal_state_| to be set in OnLocalStatePrefServiceInitialized
// before initializing DisplayPreferences (and therefore DisplayManager).
// http://crbug.com/678949.
shell_delegate_->PreInit();
bool display_initialized = display_manager_->InitFromCommandLine();
if (!display_initialized && config != Config::CLASSIC) {
// Run display configuration off device in mus mode.
display_manager_->set_configure_displays(true);
display_configurator_->set_configure_display(true);
}
display_configuration_controller_ =
std::make_unique<DisplayConfigurationController>(
display_manager_.get(), window_tree_host_manager_.get());
display_configurator_->Init(shell_port_->CreateNativeDisplayDelegate(),
false);
// The DBusThreadManager must outlive this Shell. See the DCHECK in ~Shell.
chromeos::DBusThreadManager* dbus_thread_manager =
chromeos::DBusThreadManager::Get();
projecting_observer_.reset(
new ProjectingObserver(dbus_thread_manager->GetPowerManagerClient()));
display_configurator_->AddObserver(projecting_observer_.get());
AddShellObserver(projecting_observer_.get());
if (!display_initialized) {
if (config != Config::CLASSIC && !chromeos::IsRunningAsSystemCompositor()) {
display::mojom::DevDisplayControllerPtr controller;
shell_delegate_->GetShellConnector()->BindInterface(
ui::mojom::kServiceName, &controller);
display_manager_->SetDevDisplayController(std::move(controller));
}
if (config != Config::CLASSIC || chromeos::IsRunningAsSystemCompositor()) {
display_change_observer_ =
std::make_unique<display::DisplayChangeObserver>(
display_configurator_.get(), display_manager_.get());
shutdown_observer_ =
std::make_unique<ShutdownObserver>(display_configurator_.get());
// Register |display_change_observer_| first so that the rest of
// observer gets invoked after the root windows are configured.
display_configurator_->AddObserver(display_change_observer_.get());
display_error_observer_.reset(new DisplayErrorObserver());
display_configurator_->AddObserver(display_error_observer_.get());
display_configurator_->set_state_controller(
display_change_observer_.get());
display_configurator_->set_mirroring_controller(display_manager_.get());
display_configurator_->ForceInitialConfigure();
display_initialized = true;
}
}
display_color_manager_ =
std::make_unique<DisplayColorManager>(display_configurator_.get());
if (!display_initialized)
display_manager_->InitDefaultDisplay();
InitializeDisplayManager();
if (config == Config::CLASSIC) {
display_manager_->RefreshFontParams();
// This will initialize aura::Env which requires |display_manager_| to
// be initialized first.
aura::Env::GetInstance()->set_context_factory(context_factory);
aura::Env::GetInstance()->set_context_factory_private(
context_factory_private);
......@@ -1161,6 +1109,63 @@ void Shell::Init(ui::ContextFactory* context_factory,
user_metrics_recorder_->OnShellInitialized();
}
void Shell::InitializeDisplayManager() {
const Config config = shell_port_->GetAshConfig();
bool display_initialized = display_manager_->InitFromCommandLine();
if (!display_initialized && config != Config::CLASSIC) {
// Run display configuration off device in mus mode.
display_manager_->set_configure_displays(true);
display_configurator_->set_configure_display(true);
}
display_configuration_controller_ =
std::make_unique<DisplayConfigurationController>(
display_manager_.get(), window_tree_host_manager_.get());
display_configurator_->Init(shell_port_->CreateNativeDisplayDelegate(),
false);
projecting_observer_ =
std::make_unique<ProjectingObserver>(display_configurator_.get());
if (!display_initialized) {
if (config != Config::CLASSIC && !chromeos::IsRunningAsSystemCompositor()) {
display::mojom::DevDisplayControllerPtr controller;
shell_delegate_->GetShellConnector()->BindInterface(
ui::mojom::kServiceName, &controller);
display_manager_->SetDevDisplayController(std::move(controller));
}
if (config != Config::CLASSIC || chromeos::IsRunningAsSystemCompositor()) {
display_change_observer_ =
std::make_unique<display::DisplayChangeObserver>(
display_configurator_.get(), display_manager_.get());
display_shutdown_observer_ = std::make_unique<DisplayShutdownObserver>(
display_configurator_.get());
// Register |display_change_observer_| first so that the rest of
// observer gets invoked after the root windows are configured.
display_configurator_->AddObserver(display_change_observer_.get());
display_error_observer_.reset(new DisplayErrorObserver());
display_configurator_->AddObserver(display_error_observer_.get());
display_configurator_->set_state_controller(
display_change_observer_.get());
display_configurator_->set_mirroring_controller(display_manager_.get());
display_configurator_->ForceInitialConfigure();
display_initialized = true;
}
}
display_color_manager_ =
std::make_unique<DisplayColorManager>(display_configurator_.get());
if (!display_initialized)
display_manager_->InitDefaultDisplay();
if (config == Config::CLASSIC)
display_manager_->RefreshFontParams();
}
void Shell::InitRootWindow(aura::Window* root_window) {
DCHECK(focus_controller_);
DCHECK(visibility_controller_.get());
......
......@@ -94,6 +94,7 @@ class CastConfigController;
class DisplayColorManager;
class DisplayConfigurationController;
class DisplayErrorObserver;
class DisplayShutdownObserver;
class DragDropController;
class EventClientImpl;
class EventTransformationHandler;
......@@ -143,7 +144,6 @@ class ShellDelegate;
struct ShellInitParams;
class ShellObserver;
class ShutdownController;
class ShutdownObserver;
class SmsObserver;
class SplitViewController;
class StickyKeysController;
......@@ -588,6 +588,9 @@ class ASH_EXPORT Shell : public SessionObserver,
void Init(ui::ContextFactory* context_factory,
ui::ContextFactoryPrivate* context_factory_private);
// Initializes the display manager and related components.
void InitializeDisplayManager();
// Initializes the root window so that it can host browser windows.
void InitRootWindow(aura::Window* root_window);
......@@ -745,7 +748,7 @@ class ASH_EXPORT Shell : public SessionObserver,
std::unique_ptr<display::DisplayChangeObserver> display_change_observer_;
// Listens for shutdown and updates DisplayConfigurator.
std::unique_ptr<ShutdownObserver> shutdown_observer_;
std::unique_ptr<DisplayShutdownObserver> display_shutdown_observer_;
// Listens for new sms messages and shows notifications.
std::unique_ptr<SmsObserver> sms_observer_;
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment