Commit 5dd6e801 authored by Wei Li's avatar Wei Li Committed by Commit Bot

[HaTS] Preload survey dialog before prompt

Since we fetch survey content from a remote server, we have to
handle situations such as no internet or slow connections etc. To
work around all such problems, this CL creates the web dialog and
preload the survey content before showing user the bubble banner.
We only show the bubble banner after we already retrieve the survey
content. The pros of this approach include 1) avoid handling offline
and slow loading scenarios; 2) fast UI -- the dialog will immediately
show up once user consents.

BUG=979530,996542

Change-Id: I7163c1fd06a479460d4705458c880f5e17ce1b5f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1745710
Commit-Queue: Wei Li <weili@chromium.org>
Reviewed-by: default avatarScott Violet <sky@chromium.org>
Cr-Commit-Position: refs/heads/master@{#690863}
parent 594f93b4
......@@ -124,9 +124,13 @@ class HatsServiceProbabilityOne : public HatsServiceBrowserTestBase {
// injected below.
ASSERT_FALSE(
HatsServiceFactory::GetForProfile(browser()->profile(), false));
// TODO(weili): refactor to use constants from hats_service.cc for these
// parameters.
scoped_feature_list_.InitAndEnableFeatureWithParameters(
features::kHappinessTrackingSurveysForDesktop,
{{"probability", "1.000"}, {"survey", "satisfaction"}});
{{"probability", "1.000"},
{"survey", "satisfaction"},
{"en_site_id", "test_site_id"}});
// Set the profile creation time to be old enough to ensure triggering.
browser()->profile()->SetCreationTimeForTesting(
base::Time::Now() - base::TimeDelta::FromDays(45));
......
......@@ -27,8 +27,9 @@
namespace chrome {
namespace {
gfx::NativeWindow ShowWebDialogWidget(views::Widget::InitParams params,
views::WebDialogView* view) {
gfx::NativeWindow CreateWebDialogWidget(views::Widget::InitParams params,
views::WebDialogView* view,
bool show = true) {
views::Widget* widget = new views::Widget;
widget->Init(std::move(params));
......@@ -37,7 +38,8 @@ gfx::NativeWindow ShowWebDialogWidget(views::Widget::InitParams params,
extensions::ChromeExtensionWebContentsObserver::CreateForWebContents(
view->web_contents());
widget->Show();
if (show)
widget->Show();
return widget->GetNativeWindow();
}
......@@ -68,7 +70,7 @@ gfx::NativeWindow ShowWebDialogWithParams(
ash_util::SetupWidgetInitParamsForContainer(&params, container_id);
}
#endif
gfx::NativeWindow window = ShowWebDialogWidget(std::move(params), view);
gfx::NativeWindow window = CreateWebDialogWidget(std::move(params), view);
#if defined(OS_CHROMEOS)
const user_manager::User* user =
chromeos::ProfileHelper::Get()->GetUserByProfile(
......@@ -84,10 +86,11 @@ gfx::NativeWindow ShowWebDialogWithParams(
return window;
}
gfx::NativeWindow ShowWebDialogWithBounds(gfx::NativeView parent,
content::BrowserContext* context,
ui::WebDialogDelegate* delegate,
const gfx::Rect& bounds) {
gfx::NativeWindow CreateWebDialogWithBounds(gfx::NativeView parent,
content::BrowserContext* context,
ui::WebDialogDelegate* delegate,
const gfx::Rect& bounds,
bool show) {
// Use custom dialog frame instead of platform frame when possible.
bool use_dialog_frame = views::DialogDelegate::CanSupportCustomFrame(parent);
views::WebDialogView* view = new views::WebDialogView(
......@@ -108,8 +111,7 @@ gfx::NativeWindow ShowWebDialogWithBounds(gfx::NativeView parent,
#endif
}
gfx::NativeWindow window = ShowWebDialogWidget(std::move(params), view);
return window;
return CreateWebDialogWidget(std::move(params), view, show);
}
} // namespace chrome
......@@ -28,12 +28,14 @@ gfx::NativeWindow ShowWebDialogWithParams(
// The implementation is more aligned with the appearance of constrained
// web dialog.
// |show| indicates whether to show the web dialog after it is created.
// TODO(weili): Solely use this function on non-ChromeOS platform, and
// above ShowWebDialogWithParams() on ChromeOS. Or merge these two if possible.
gfx::NativeWindow ShowWebDialogWithBounds(gfx::NativeView parent,
content::BrowserContext* context,
ui::WebDialogDelegate* delegate,
const gfx::Rect& bounds);
gfx::NativeWindow CreateWebDialogWithBounds(gfx::NativeView parent,
content::BrowserContext* context,
ui::WebDialogDelegate* delegate,
const gfx::Rect& bounds,
bool show = true);
} // namespace chrome
......
......@@ -3038,7 +3038,7 @@ void BrowserView::ShowAvatarBubbleFromAvatarButton(
}
void BrowserView::ShowHatsBubble(const std::string& site_id) {
HatsBubbleView::Show(browser(), site_id);
HatsBubbleView::ShowOnContentReady(browser(), site_id);
}
void BrowserView::ExecuteExtensionCommand(
......
......@@ -5,10 +5,18 @@
#include <memory>
#include <string>
#include "base/files/file_path.h"
#include "base/path_service.h"
#include "base/run_loop.h"
#include "base/time/time.h"
#include "chrome/browser/ui/browser.h"
#include "chrome/browser/ui/test/test_browser_dialog.h"
#include "chrome/browser/ui/views/frame/browser_view.h"
#include "chrome/browser/ui/views/hats/hats_bubble_view.h"
#include "chrome/browser/ui/views/hats/hats_web_dialog.h"
#include "chrome/common/chrome_paths.h"
#include "testing/gmock/include/gmock/gmock.h"
#include "url/gurl.h"
class HatsBubbleTest : public DialogBrowserTest {
public:
......@@ -18,14 +26,81 @@ class HatsBubbleTest : public DialogBrowserTest {
void ShowUi(const std::string& name) override {
ASSERT_TRUE(browser()->is_type_normal());
BrowserView::GetBrowserViewForBrowser(InProcessBrowserTest::browser())
->ShowHatsBubble(std::string());
->ShowHatsBubble("test_site_id");
}
private:
DISALLOW_COPY_AND_ASSIGN(HatsBubbleTest);
};
class TestHatsWebDialog : public HatsWebDialog {
public:
TestHatsWebDialog(Browser* browser,
const base::TimeDelta& timeout,
const GURL& url)
: HatsWebDialog(browser, "fake_id_not_used"),
loading_timeout_(timeout),
content_url_(url) {}
// ui::WebDialogDelegate implementation.
GURL GetDialogContentURL() const override {
if (content_url_.is_valid()) {
// When we have a valid overridden url, use it instead.
return content_url_;
}
return HatsWebDialog::GetDialogContentURL();
}
MOCK_METHOD0(OnWebContentsFinishedLoad, void());
MOCK_METHOD0(OnLoadTimedOut, void());
private:
const base::TimeDelta ContentLoadingTimeout() const override {
return loading_timeout_;
}
base::TimeDelta loading_timeout_;
GURL content_url_;
};
class HatsWebDialogBrowserTest : public InProcessBrowserTest {
public:
TestHatsWebDialog* Create(Browser* browser,
const base::TimeDelta& timeout,
const GURL& url = GURL()) {
auto* hats_dialog = new TestHatsWebDialog(browser, timeout, url);
hats_dialog->CreateWebDialog(browser);
return hats_dialog;
}
};
// Test that calls ShowUi("default").
IN_PROC_BROWSER_TEST_F(HatsBubbleTest, InvokeUi_default) {
IN_PROC_BROWSER_TEST_F(HatsBubbleTest, InvokeUi_Default) {
ShowAndVerifyUi();
}
// Test time out of preloading works.
IN_PROC_BROWSER_TEST_F(HatsWebDialogBrowserTest, Timeout) {
TestHatsWebDialog* dialog = Create(browser(), base::TimeDelta());
EXPECT_CALL(*dialog, OnLoadTimedOut).Times(1);
}
// Test preloading content works.
IN_PROC_BROWSER_TEST_F(HatsWebDialogBrowserTest, ContentPreloading) {
base::FilePath test_data_dir;
base::PathService::Get(chrome::DIR_TEST_DATA, &test_data_dir);
std::string contents;
{
base::ScopedAllowBlockingForTesting allow_blocking;
EXPECT_TRUE(base::ReadFileToString(test_data_dir.AppendASCII("simple.html"),
&contents));
}
TestHatsWebDialog* dialog =
Create(browser(), base::TimeDelta::FromSeconds(100),
GURL("data:text/html;charset=utf-8," + contents));
base::RunLoop run_loop;
EXPECT_CALL(*dialog, OnWebContentsFinishedLoad)
.WillOnce(testing::Invoke(&run_loop, &base::RunLoop::Quit));
run_loop.Run();
}
......@@ -4,6 +4,7 @@
#include "chrome/browser/ui/views/hats/hats_bubble_view.h"
#include "base/bind_helpers.h"
#include "base/metrics/histogram_macros.h"
#include "base/task/post_task.h"
#include "chrome/browser/ui/browser.h"
......@@ -57,8 +58,23 @@ views::BubbleDialogDelegateView* HatsBubbleView::GetHatsBubble() {
}
// static
void HatsBubbleView::ShowOnContentReady(Browser* browser,
const std::string& site_id) {
if (site_id == "test_site_id") {
// Directly show the bubble during tests.
HatsBubbleView::Show(browser, base::DoNothing());
return;
}
// Try to create the web dialog and preload the HaTS survey content first.
// The bubble will only show after the survey content is retrieved.
// If it fails due to no internet connection or any other reason, the bubble
// will not show.
HatsWebDialog::Create(browser, site_id);
}
void HatsBubbleView::Show(Browser* browser,
const std::string& site_id) {
HatsConsentCallback consent_callback) {
AppMenuButton* anchor_button = BrowserView::GetBrowserViewForBrowser(browser)
->toolbar_button_provider()
->GetAppMenuButton();
......@@ -70,20 +86,19 @@ void HatsBubbleView::Show(Browser* browser,
gfx::NativeView parent_view = anchor_button->GetWidget()->GetNativeView();
// Bubble delegate will be deleted when its window is destroyed.
auto* bubble =
new HatsBubbleView(browser, anchor_button, site_id, parent_view);
auto* bubble = new HatsBubbleView(browser, anchor_button, parent_view,
std::move(consent_callback));
bubble->SetHighlightedButton(anchor_button);
bubble->GetWidget()->Show();
}
HatsBubbleView::HatsBubbleView(Browser* browser,
AppMenuButton* anchor_button,
const std::string& site_id,
gfx::NativeView parent_view)
gfx::NativeView parent_view,
HatsConsentCallback consent_callback)
: BubbleDialogDelegateView(anchor_button, views::BubbleBorder::TOP_RIGHT),
close_bubble_helper_(this, browser),
site_id_(site_id),
browser_(browser) {
consent_callback_(std::move(consent_callback)) {
chrome::RecordDialogCreation(chrome::DialogIdentifier::HATS_BUBBLE);
set_close_on_deactivate(false);
......@@ -104,7 +119,11 @@ HatsBubbleView::HatsBubbleView(Browser* browser,
instance_ = this;
}
HatsBubbleView::~HatsBubbleView() {}
HatsBubbleView::~HatsBubbleView() {
// If the callback was not run before, we need to run it now.
if (consent_callback_)
std::move(consent_callback_).Run(false);
}
base::string16 HatsBubbleView::GetWindowTitle() const {
return l10n_util::GetStringUTF16(IDS_HATS_BUBBLE_TITLE);
......@@ -126,8 +145,15 @@ base::string16 HatsBubbleView::GetDialogButtonLabel(
: l10n_util::GetStringUTF16(IDS_NO_THANKS);
}
bool HatsBubbleView::Cancel() {
if (consent_callback_)
std::move(consent_callback_).Run(false);
return true;
}
bool HatsBubbleView::Accept() {
HatsWebDialog::Show(browser_, site_id_);
if (consent_callback_)
std::move(consent_callback_).Run(true);
return true;
}
......
......@@ -16,6 +16,11 @@
class AppMenuButton;
class Browser;
// The type of the callback invoked when a user consents, cancels, or dismisses
// the consent banner bubble.
// |accept| indicates whether the user consents to take the survey.
using HatsConsentCallback = base::OnceCallback<void(bool accept)>;
// This bubble view is displayed when a Happiness tracking survey is triggered.
// It displays a WebUI that hosts the survey.
class HatsBubbleView : public views::BubbleDialogDelegateView {
......@@ -33,9 +38,12 @@ class HatsBubbleView : public views::BubbleDialogDelegateView {
// Returns a pointer to the Hats Bubble being shown. For testing only.
static views::BubbleDialogDelegateView* GetHatsBubble();
// Creates and shows the bubble that leads to a survey identified by
// |site_id|.
static void Show(Browser* browser, const std::string& site_id);
// Shows the bubble when the survey content identified by |site_id| is ready.
static void ShowOnContentReady(Browser* browser, const std::string& site_id);
// Shows the bubble now with supplied callback |consent_callback|.
static void Show(Browser* browser, HatsConsentCallback consent_callback);
protected:
// views::WidgetDelegate:
......@@ -46,6 +54,7 @@ class HatsBubbleView : public views::BubbleDialogDelegateView {
// views::DialogDelegate:
base::string16 GetDialogButtonLabel(ui::DialogButton button) const override;
bool Cancel() override;
bool Accept() override;
// views::WidgetObserver:
......@@ -58,14 +67,13 @@ class HatsBubbleView : public views::BubbleDialogDelegateView {
private:
HatsBubbleView(Browser* browser,
AppMenuButton* anchor_button,
const std::string& site_id,
gfx::NativeView parent_view);
gfx::NativeView parent_view,
HatsConsentCallback consent_callback);
~HatsBubbleView() override;
static HatsBubbleView* instance_;
CloseBubbleOnTabActivationHelper close_bubble_helper_;
const std::string site_id_;
const Browser* browser_;
HatsConsentCallback consent_callback_;
DISALLOW_COPY_AND_ASSIGN(HatsBubbleView);
};
......
......@@ -12,6 +12,7 @@
#include "chrome/browser/ui/browser.h"
#include "chrome/browser/ui/views/chrome_web_dialog_view.h"
#include "chrome/browser/ui/views/frame/browser_view.h"
#include "chrome/browser/ui/views/hats/hats_bubble_view.h"
#include "chrome/browser/ui/views/location_bar/location_bar_view.h"
#include "chrome/browser/ui/views/toolbar/toolbar_view.h"
#include "chrome/grit/browser_resources.h"
......@@ -70,38 +71,23 @@ std::string LoadLocalHtmlAsString(const std::string& site_id,
} // namespace
// static
void HatsWebDialog::Show(const Browser* browser, const std::string& site_id) {
void HatsWebDialog::Create(Browser* browser, const std::string& site_id) {
DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
DCHECK(browser);
Profile* profile = browser->profile();
// Self deleting upon close.
auto* hats_dialog = new HatsWebDialog(profile, site_id);
// Create a web dialog aligned to the bottom center of the location bar.
BrowserView* browser_view = BrowserView::GetBrowserViewForBrowser(browser);
LocationBarView* location_bar = browser_view->GetLocationBarView();
DCHECK(location_bar);
gfx::Rect bounds(location_bar->bounds());
views::View::ConvertRectToScreen(browser_view->toolbar(), &bounds);
bounds = gfx::Rect(
bounds.x() +
std::max(0, bounds.width() / 2 - kDefaultHatsDialogWidth / 2),
bounds.bottom() - views::BubbleBorder::GetBorderAndShadowInsets().top(),
kDefaultHatsDialogWidth, kDefaultHatsDialogHeight);
chrome::ShowWebDialogWithBounds(browser_view->GetWidget()->GetNativeView(),
hats_dialog->off_the_record_profile(),
hats_dialog, bounds);
auto* hats_dialog = new HatsWebDialog(browser, site_id);
hats_dialog->CreateWebDialog(browser);
}
HatsWebDialog::HatsWebDialog(Profile* profile, const std::string& site_id)
HatsWebDialog::HatsWebDialog(Browser* browser, const std::string& site_id)
: otr_profile_registration_(
IndependentOTRProfileManager::GetInstance()
->CreateFromOriginalProfile(
profile,
browser->profile(),
base::BindOnce(&HatsWebDialog::OnOriginalProfileDestroyed,
base::Unretained(this)))),
browser_(browser),
site_id_(site_id) {
DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
DCHECK(
......@@ -165,8 +151,70 @@ bool HatsWebDialog::HandleContextMenu(
return true;
}
void HatsWebDialog::OnWebContentsFinishedLoad() {
// If this happens after the time out, the dialog is to be deleted. We
// should not handle this any more.
if (!loading_timer_.IsRunning())
return;
loading_timer_.Stop();
HatsBubbleView::Show(
browser_, base::BindOnce(&HatsWebDialog::Show, weak_factory_.GetWeakPtr(),
preloading_widget_));
}
void HatsWebDialog::OnLoadTimedOut() {
// Once loading is timed out, it means there is some problem such as network
// error, unresponsive server etc. No need to wait any longer. Delete the
// dialog.
preloading_widget_->Close();
}
const base::TimeDelta HatsWebDialog::ContentLoadingTimeout() const {
// Time out for loading the survey content in the dialog.
static const base::TimeDelta kLoadingTimeOut =
base::TimeDelta::FromSeconds(10);
return kLoadingTimeOut;
}
void HatsWebDialog::CreateWebDialog(Browser* browser) {
// Create a web dialog aligned to the bottom center of the location bar.
BrowserView* browser_view = BrowserView::GetBrowserViewForBrowser(browser);
LocationBarView* location_bar = browser_view->GetLocationBarView();
DCHECK(location_bar);
gfx::Rect bounds(location_bar->bounds());
views::View::ConvertRectToScreen(browser_view->toolbar(), &bounds);
bounds = gfx::Rect(
bounds.x() +
std::max(0, bounds.width() / 2 - kDefaultHatsDialogWidth / 2),
bounds.bottom() - views::BubbleBorder::GetBorderAndShadowInsets().top(),
kDefaultHatsDialogWidth, kDefaultHatsDialogHeight);
gfx::NativeWindow native_window = chrome::CreateWebDialogWithBounds(
browser_view->GetWidget()->GetNativeView(), off_the_record_profile(),
this, bounds,
/*show=*/false);
preloading_widget_ = views::Widget::GetWidgetForNativeWindow(native_window);
// Start the loading timer once it is created.
loading_timer_.Start(FROM_HERE, ContentLoadingTimeout(),
base::BindOnce(&HatsWebDialog::OnLoadTimedOut,
weak_factory_.GetWeakPtr()));
}
void HatsWebDialog::OnOriginalProfileDestroyed(Profile* profile) {
if (otr_profile_registration_ &&
profile == otr_profile_registration_->profile())
otr_profile_registration_.reset();
}
void HatsWebDialog::Show(views::Widget* widget, bool accept) {
if (accept) {
if (widget)
widget->Show();
return;
}
// Delete this dialog.
widget->Close();
}
......@@ -9,32 +9,36 @@
#include <string>
#include "base/macros.h"
#include "base/timer/timer.h"
#include "chrome/browser/media/router/presentation/independent_otr_profile_manager.h"
#include "ui/web_dialogs/web_dialog_delegate.h"
class Browser;
class Profile;
namespace views {
class Widget;
}
// Happiness tracking survey dialog which shows the survey content.
// This class lives on the UI thread and is self deleting.
// TODO(weili): This dialog shares a lot of common code with the one in
// chrome/browser/chromeos/hats/, should be merged into one.
class HatsWebDialog : public ui::WebDialogDelegate {
public:
// Create and show an instance of HatsWebDialog.
static void Show(const Browser* browser, const std::string& site_id);
// Create an instance of HatsWebDialog and load its content without showing.
static void Create(Browser* browser, const std::string& site_id);
private:
// Use Show() above. An off the record profile is created from the given
friend class TestHatsWebDialog;
friend class HatsWebDialogBrowserTest;
// Use Create() above. An off the record profile is created from the given
// browser profile which is used for navigating to the survey. |site_id| is
// used to select the survey.
explicit HatsWebDialog(Profile* profile, const std::string& site_id);
HatsWebDialog(Browser* browser, const std::string& site_id);
~HatsWebDialog() override;
Profile* off_the_record_profile() {
return otr_profile_registration_->profile();
}
// ui::WebDialogDelegate implementation.
ui::ModalType GetDialogModalType() const override;
base::string16 GetDialogTitle() const override;
......@@ -51,13 +55,34 @@ class HatsWebDialog : public ui::WebDialogDelegate {
bool ShouldShowDialogTitle() const override;
bool HandleContextMenu(content::RenderFrameHost* render_frame_host,
const content::ContextMenuParams& params) override;
void OnWebContentsFinishedLoad() override;
// These are virtual for tests.
virtual void OnLoadTimedOut();
virtual const base::TimeDelta ContentLoadingTimeout() const;
Profile* off_the_record_profile() {
return otr_profile_registration_->profile();
}
void CreateWebDialog(Browser* browser);
void OnOriginalProfileDestroyed(Profile* profile);
void Show(views::Widget* widget, bool accept);
std::unique_ptr<IndependentOTRProfileManager::OTRProfileRegistration>
otr_profile_registration_;
Browser* browser_;
const std::string site_id_;
// A timer to prevent unresponsive loading of survey dialog.
base::OneShotTimer loading_timer_;
// The widget created for preloading. It is owned by us until it is shown to
// user.
views::Widget* preloading_widget_{nullptr};
base::WeakPtrFactory<HatsWebDialog> weak_factory_{this};
DISALLOW_COPY_AND_ASSIGN(HatsWebDialog);
};
......
......@@ -35,6 +35,23 @@ using ui::WebDialogWebContentsDelegate;
namespace views {
ObservableWebView::ObservableWebView(content::BrowserContext* browser_context,
WebDialogDelegate* delegate)
: WebView(browser_context), delegate_(delegate) {}
ObservableWebView::~ObservableWebView() {}
void ObservableWebView::DidFinishLoad(
content::RenderFrameHost* render_frame_host,
const GURL& validated_url) {
// Only listen to the main frame.
if (render_frame_host->GetParent())
return;
if (delegate_)
delegate_->OnWebContentsFinishedLoad();
}
////////////////////////////////////////////////////////////////////////////////
// WebDialogView, public:
......@@ -45,7 +62,7 @@ WebDialogView::WebDialogView(content::BrowserContext* context,
: ClientView(nullptr, nullptr),
WebDialogWebContentsDelegate(context, std::move(handler)),
delegate_(delegate),
web_view_(new views::WebView(context)),
web_view_(new ObservableWebView(context, delegate)),
use_dialog_frame_(use_dialog_frame) {
web_view_->set_allow_accelerators(true);
AddChildView(web_view_);
......
......@@ -15,6 +15,7 @@
#include "base/macros.h"
#include "ui/gfx/geometry/size.h"
#include "ui/views/controls/webview/unhandled_keyboard_event_handler.h"
#include "ui/views/controls/webview/webview.h"
#include "ui/views/controls/webview/webview_export.h"
#include "ui/views/widget/widget_delegate.h"
#include "ui/views/window/client_view.h"
......@@ -23,10 +24,27 @@
namespace content {
class BrowserContext;
class RenderFrameHost;
}
namespace views {
class WebView;
// A kind of webview that can notify its delegate when its content is ready.
class ObservableWebView : public WebView {
public:
ObservableWebView(content::BrowserContext* browser_context,
ui::WebDialogDelegate* delegate);
~ObservableWebView() override;
// content::WebContentsObserver
void DidFinishLoad(content::RenderFrameHost* render_frame_host,
const GURL& validated_url) override;
private:
ui::WebDialogDelegate* delegate_;
DISALLOW_COPY_AND_ASSIGN(ObservableWebView);
};
////////////////////////////////////////////////////////////////////////////////
//
......@@ -149,7 +167,7 @@ class WEBVIEW_EXPORT WebDialogView : public views::ClientView,
// using this variable.
ui::WebDialogDelegate* delegate_;
views::WebView* web_view_;
ObservableWebView* web_view_;
// Whether user is attempting to close the dialog and we are processing
// beforeunload event.
......
......@@ -153,6 +153,8 @@ class WEB_DIALOGS_EXPORT WebDialogDelegate {
// Returns true if |accelerator| is processed, otherwise false.
virtual bool AcceleratorPressed(const Accelerator& accelerator);
virtual void OnWebContentsFinishedLoad() {}
virtual ~WebDialogDelegate() {}
};
......
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