Commit e0eb9045 authored by Clemens Arbesser's avatar Clemens Arbesser Committed by Commit Bot

[Autofill Assistant] Extended the amount of information available through feedback forms.

The stock android feedback form has an optional string field `feedbackContext', which can be used to provide extended information. Depending on when the user taps the feedback button, not all of the information may be available.

Bug: 806868
Change-Id: I5dfaaf813c48198f573f2e1fbd0d6468439d3a6c
Reviewed-on: https://chromium-review.googlesource.com/c/1317911
Commit-Queue: Clemens Arbesser <arbesser@google.com>
Reviewed-by: default avatarStephane Zermatten <szermatt@chromium.org>
Cr-Commit-Position: refs/heads/master@{#606823}
parent c01e9b4c
...@@ -182,6 +182,11 @@ public class AutofillAssistantUiController implements AutofillAssistantUiDelegat ...@@ -182,6 +182,11 @@ public class AutofillAssistantUiController implements AutofillAssistantUiDelegat
nativeOnCardSelected(mUiControllerAndroid, guid); nativeOnCardSelected(mUiControllerAndroid, guid);
} }
@Override
public String getDebugContext() {
return nativeOnRequestDebugContext(mUiControllerAndroid);
}
@Override @Override
public boolean allowTouchEvent(float x, float y) { public boolean allowTouchEvent(float x, float y) {
return nativeAllowTouchEvent(mUiControllerAndroid, x, y); return nativeAllowTouchEvent(mUiControllerAndroid, x, y);
...@@ -591,5 +596,6 @@ public class AutofillAssistantUiController implements AutofillAssistantUiDelegat ...@@ -591,5 +596,6 @@ public class AutofillAssistantUiController implements AutofillAssistantUiDelegat
private native void nativeOnAccessToken( private native void nativeOnAccessToken(
long nativeUiControllerAndroid, boolean success, String accessToken); long nativeUiControllerAndroid, boolean success, String accessToken);
private native String nativeGetPrimaryAccountName(long nativeUiControllerAndroid); private native String nativeGetPrimaryAccountName(long nativeUiControllerAndroid);
private native String nativeOnRequestDebugContext(long nativeUiControllerAndroid);
private native boolean nativeAllowTouchEvent(long nativeUiControllerAndroid, float x, float y); private native boolean nativeAllowTouchEvent(long nativeUiControllerAndroid, float x, float y);
} }
...@@ -32,6 +32,8 @@ import android.widget.ImageView; ...@@ -32,6 +32,8 @@ import android.widget.ImageView;
import android.widget.LinearLayout; import android.widget.LinearLayout;
import android.widget.TextView; import android.widget.TextView;
import org.json.JSONObject;
import org.chromium.chrome.autofill_assistant.R; import org.chromium.chrome.autofill_assistant.R;
import org.chromium.chrome.browser.ChromeActivity; import org.chromium.chrome.browser.ChromeActivity;
import org.chromium.chrome.browser.autofill.PersonalDataManager.AutofillProfile; import org.chromium.chrome.browser.autofill.PersonalDataManager.AutofillProfile;
...@@ -50,8 +52,10 @@ import java.text.SimpleDateFormat; ...@@ -50,8 +52,10 @@ import java.text.SimpleDateFormat;
import java.util.ArrayList; import java.util.ArrayList;
import java.util.Collections; import java.util.Collections;
import java.util.Date; import java.util.Date;
import java.util.HashMap;
import java.util.List; import java.util.List;
import java.util.Locale; import java.util.Locale;
import java.util.Map;
/** Delegate to interact with the assistant UI. */ /** Delegate to interact with the assistant UI. */
class AutofillAssistantUiDelegate { class AutofillAssistantUiDelegate {
...@@ -78,7 +82,7 @@ class AutofillAssistantUiDelegate { ...@@ -78,7 +82,7 @@ class AutofillAssistantUiDelegate {
private final TextView mStatusMessageView; private final TextView mStatusMessageView;
private final AnimatedProgressBar mProgressBar; private final AnimatedProgressBar mProgressBar;
private final ViewGroup mDetails; private final ViewGroup mDetailsViewContainer;
private final ImageView mDetailsImage; private final ImageView mDetailsImage;
private final TextView mDetailsTitle; private final TextView mDetailsTitle;
private final TextView mDetailsText; private final TextView mDetailsText;
...@@ -89,6 +93,10 @@ class AutofillAssistantUiDelegate { ...@@ -89,6 +93,10 @@ class AutofillAssistantUiDelegate {
private final boolean mIsRightToLeftLayout; private final boolean mIsRightToLeftLayout;
private ValueAnimator mDetailsPulseAnimation = null; private ValueAnimator mDetailsPulseAnimation = null;
@Nullable
private Details mDetails;
private String mStatusMessage;
/** /**
* This is a client interface that relays interactions from the UI. * This is a client interface that relays interactions from the UI.
* *
...@@ -126,6 +134,13 @@ class AutofillAssistantUiDelegate { ...@@ -126,6 +134,13 @@ class AutofillAssistantUiDelegate {
*/ */
void onCardSelected(String guid); void onCardSelected(String guid);
/**
* Used to access information relevant for debugging purposes.
*
* @return A string describing the current execution context.
*/
String getDebugContext();
/** /**
* Called when the init was successful. * Called when the init was successful.
*/ */
...@@ -203,6 +218,16 @@ class AutofillAssistantUiDelegate { ...@@ -203,6 +218,16 @@ class AutofillAssistantUiDelegate {
return mDescription; return mDescription;
} }
JSONObject toJSONObject() {
// Details are part of the feedback form, hence they need a JSON representation.
Map<String, String> movieDetails = new HashMap<>();
movieDetails.put("title", mTitle);
movieDetails.put("url", mUrl);
if (mDate != null) movieDetails.put("date", mDate.toString());
movieDetails.put("description", mDescription);
return new JSONObject(movieDetails);
}
/** /**
* Whether the details are not subject to change anymore. If set to false the animated * Whether the details are not subject to change anymore. If set to false the animated
* placeholders will be displayed in place of missing data. * placeholders will be displayed in place of missing data.
...@@ -247,7 +272,9 @@ class AutofillAssistantUiDelegate { ...@@ -247,7 +272,9 @@ class AutofillAssistantUiDelegate {
.setOnClickListener(unusedView .setOnClickListener(unusedView
-> HelpAndFeedback.getInstance(mActivity).showFeedback(mActivity, -> HelpAndFeedback.getInstance(mActivity).showFeedback(mActivity,
Profile.getLastUsedProfile(), mActivity.getActivityTab().getUrl(), Profile.getLastUsedProfile(), mActivity.getActivityTab().getUrl(),
FEEDBACK_CATEGORY_TAG)); FEEDBACK_CATEGORY_TAG,
FeedbackContext.buildContextString(
mActivity, mClient, mDetails, mStatusMessage, 4)));
mCarouselScroll = mBottomBar.findViewById(R.id.carousel_scroll); mCarouselScroll = mBottomBar.findViewById(R.id.carousel_scroll);
mChipsViewContainer = mCarouselScroll.findViewById(R.id.carousel); mChipsViewContainer = mCarouselScroll.findViewById(R.id.carousel);
mStatusMessageView = mBottomBar.findViewById(R.id.status_message); mStatusMessageView = mBottomBar.findViewById(R.id.status_message);
...@@ -255,17 +282,17 @@ class AutofillAssistantUiDelegate { ...@@ -255,17 +282,17 @@ class AutofillAssistantUiDelegate {
mActivity.getColor(R.color.modern_blue_600), mActivity.getColor(R.color.modern_blue_600),
mActivity.getColor(R.color.modern_blue_600_alpha_38_opaque)); mActivity.getColor(R.color.modern_blue_600_alpha_38_opaque));
mDetails = (ViewGroup) mBottomBar.findViewById(R.id.details); mDetailsViewContainer = (ViewGroup) mBottomBar.findViewById(R.id.details);
mDetailsImage = mDetails.findViewById(R.id.details_image); mDetailsImage = mDetailsViewContainer.findViewById(R.id.details_image);
mDetailsTitle = (TextView) mDetails.findViewById(R.id.details_title); mDetailsTitle = (TextView) mDetailsViewContainer.findViewById(R.id.details_title);
mDetailsText = (TextView) mDetails.findViewById(R.id.details_text); mDetailsText = (TextView) mDetailsViewContainer.findViewById(R.id.details_text);
mDetailsImageWidth = mActivity.getResources().getDimensionPixelSize( mDetailsImageWidth = mActivity.getResources().getDimensionPixelSize(
R.dimen.autofill_assistant_details_image_size); R.dimen.autofill_assistant_details_image_size);
mDetailsImageHeight = mActivity.getResources().getDimensionPixelSize( mDetailsImageHeight = mActivity.getResources().getDimensionPixelSize(
R.dimen.autofill_assistant_details_image_size); R.dimen.autofill_assistant_details_image_size);
mBottomBarAnimations = new BottomBarAnimations(mBottomBar, mDetails, mChipsViewContainer, mBottomBarAnimations = new BottomBarAnimations(mBottomBar, mDetailsViewContainer,
mActivity.getResources().getDisplayMetrics()); mChipsViewContainer, mActivity.getResources().getDisplayMetrics());
mIsRightToLeftLayout = TextUtilsCompat.getLayoutDirectionFromLocale(Locale.getDefault()) mIsRightToLeftLayout = TextUtilsCompat.getLayoutDirectionFromLocale(Locale.getDefault())
== ViewCompat.LAYOUT_DIRECTION_RTL; == ViewCompat.LAYOUT_DIRECTION_RTL;
...@@ -288,6 +315,9 @@ class AutofillAssistantUiDelegate { ...@@ -288,6 +315,9 @@ class AutofillAssistantUiDelegate {
* @param message Message to display. * @param message Message to display.
*/ */
public void showStatusMessage(@Nullable String message) { public void showStatusMessage(@Nullable String message) {
/// keep a copy of the most recent status message for feedback purposes
mStatusMessage = message;
show(); show();
mStatusMessageView.setText(message); mStatusMessageView.setText(message);
...@@ -468,6 +498,9 @@ class AutofillAssistantUiDelegate { ...@@ -468,6 +498,9 @@ class AutofillAssistantUiDelegate {
/** Called to show contextual information. */ /** Called to show contextual information. */
public void showDetails(Details details) { public void showDetails(Details details) {
// keep a copy of most recent movie details for feedback purposes
mDetails = details;
Drawable defaultImage = AppCompatResources.getDrawable( Drawable defaultImage = AppCompatResources.getDrawable(
mActivity, R.drawable.autofill_assistant_default_details); mActivity, R.drawable.autofill_assistant_default_details);
......
// Copyright 2018 The Chromium Authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
package org.chromium.chrome.browser.autofill_assistant;
import org.json.JSONException;
import org.json.JSONObject;
import org.chromium.chrome.browser.ChromeActivity;
import org.chromium.chrome.browser.autofill_assistant.AutofillAssistantUiDelegate.Client;
/**
* Automatically extracts context information and serializes it in JSON form.
*/
class FeedbackContext extends JSONObject {
static String buildContextString(ChromeActivity activity, Client client,
AutofillAssistantUiDelegate.Details details, String statusMessage, int indentSpaces) {
try {
return new FeedbackContext(activity, client, details, statusMessage)
.toString(indentSpaces);
} catch (JSONException e) {
return "{\"error\": \"" + e.getMessage() + "\"}";
}
}
private FeedbackContext(ChromeActivity activity, Client client,
AutofillAssistantUiDelegate.Details details, String statusMessage)
throws JSONException {
addActivityInformation(activity);
addClientContext(client);
put("movie", details.toJSONObject());
put("status", statusMessage);
}
private void addActivityInformation(ChromeActivity activity) throws JSONException {
put("intent-action", activity.getInitialIntent().getAction());
put("intent-data", activity.getInitialIntent().getDataString());
}
private void addClientContext(Client client) throws JSONException {
// Try to parse the debug context as JSON object. If that fails, just add the string as-is.
try {
put("debug-context", new JSONObject(client.getDebugContext()));
} catch (JSONException encodingException) {
put("debug-context", client.getDebugContext());
}
}
}
...@@ -123,6 +123,7 @@ chrome_java_sources = [ ...@@ -123,6 +123,7 @@ chrome_java_sources = [
"java/src/org/chromium/chrome/browser/autofill_assistant/AutofillAssistantPaymentRequest.java", "java/src/org/chromium/chrome/browser/autofill_assistant/AutofillAssistantPaymentRequest.java",
"java/src/org/chromium/chrome/browser/autofill_assistant/AutofillAssistantUiController.java", "java/src/org/chromium/chrome/browser/autofill_assistant/AutofillAssistantUiController.java",
"java/src/org/chromium/chrome/browser/autofill_assistant/AutofillAssistantUiDelegate.java", "java/src/org/chromium/chrome/browser/autofill_assistant/AutofillAssistantUiDelegate.java",
"java/src/org/chromium/chrome/browser/autofill_assistant/FeedbackContext.java",
"java/src/org/chromium/chrome/browser/autofill_assistant/InitScreenController.java", "java/src/org/chromium/chrome/browser/autofill_assistant/InitScreenController.java",
"java/src/org/chromium/chrome/browser/autofill_assistant/ui/BottomBarAnimations.java", "java/src/org/chromium/chrome/browser/autofill_assistant/ui/BottomBarAnimations.java",
"java/src/org/chromium/chrome/browser/autofill_assistant/ui/PaymentRequestBottomBar.java", "java/src/org/chromium/chrome/browser/autofill_assistant/ui/PaymentRequestBottomBar.java",
......
...@@ -240,6 +240,13 @@ UiControllerAndroid::GetPrimaryAccountName( ...@@ -240,6 +240,13 @@ UiControllerAndroid::GetPrimaryAccountName(
return base::android::ConvertUTF8ToJavaString(env, account_info.email); return base::android::ConvertUTF8ToJavaString(env, account_info.email);
} }
base::android::ScopedJavaLocalRef<jstring>
UiControllerAndroid::OnRequestDebugContext(
JNIEnv* env,
const base::android::JavaParamRef<jobject>& jcaller) {
return base::android::ConvertUTF8ToJavaString(env, GetDebugContext());
}
jboolean UiControllerAndroid::AllowTouchEvent( jboolean UiControllerAndroid::AllowTouchEvent(
JNIEnv* env, JNIEnv* env,
const base::android::JavaParamRef<jobject>& jcaller, const base::android::JavaParamRef<jobject>& jcaller,
...@@ -320,6 +327,10 @@ void UiControllerAndroid::HideProgressBar() { ...@@ -320,6 +327,10 @@ void UiControllerAndroid::HideProgressBar() {
env, java_autofill_assistant_ui_controller_); env, java_autofill_assistant_ui_controller_);
} }
std::string UiControllerAndroid::GetDebugContext() const {
return ui_delegate_->GetDebugContext();
}
std::string UiControllerAndroid::GetApiKey() { std::string UiControllerAndroid::GetApiKey() {
std::string api_key; std::string api_key;
if (google_apis::IsGoogleChromeAPIKeyUsed()) { if (google_apis::IsGoogleChromeAPIKeyUsed()) {
......
...@@ -54,6 +54,7 @@ class UiControllerAndroid : public UiController, ...@@ -54,6 +54,7 @@ class UiControllerAndroid : public UiController,
void ShowDetails(const DetailsProto& details) override; void ShowDetails(const DetailsProto& details) override;
void ShowProgressBar(int progress, const std::string& message) override; void ShowProgressBar(int progress, const std::string& message) override;
void HideProgressBar() override; void HideProgressBar() override;
std::string GetDebugContext() const override;
// Overrides Client: // Overrides Client:
std::string GetApiKey() override; std::string GetApiKey() override;
...@@ -101,6 +102,9 @@ class UiControllerAndroid : public UiController, ...@@ -101,6 +102,9 @@ class UiControllerAndroid : public UiController,
base::android::ScopedJavaLocalRef<jstring> GetPrimaryAccountName( base::android::ScopedJavaLocalRef<jstring> GetPrimaryAccountName(
JNIEnv* env, JNIEnv* env,
const base::android::JavaParamRef<jobject>& jcaller); const base::android::JavaParamRef<jobject>& jcaller);
base::android::ScopedJavaLocalRef<jstring> OnRequestDebugContext(
JNIEnv* env,
const base::android::JavaParamRef<jobject>& jcaller);
jboolean AllowTouchEvent(JNIEnv* env, jboolean AllowTouchEvent(JNIEnv* env,
const base::android::JavaParamRef<jobject>& jcaller, const base::android::JavaParamRef<jobject>& jcaller,
float x, float x,
......
...@@ -6,8 +6,10 @@ ...@@ -6,8 +6,10 @@
#include <utility> #include <utility>
#include "base/json/json_writer.h"
#include "base/strings/utf_string_conversions.h" #include "base/strings/utf_string_conversions.h"
#include "base/task/post_task.h" #include "base/task/post_task.h"
#include "base/values.h"
#include "components/autofill_assistant/browser/protocol_utils.h" #include "components/autofill_assistant/browser/protocol_utils.h"
#include "components/autofill_assistant/browser/ui_controller.h" #include "components/autofill_assistant/browser/ui_controller.h"
#include "components/strings/grit/components_strings.h" #include "components/strings/grit/components_strings.h"
...@@ -287,6 +289,23 @@ void Controller::OnScriptSelected(const std::string& script_path) { ...@@ -287,6 +289,23 @@ void Controller::OnScriptSelected(const std::string& script_path) {
base::Unretained(this), script_path)); base::Unretained(this), script_path));
} }
std::string Controller::GetDebugContext() {
base::Value dict(base::Value::Type::DICTIONARY);
std::vector<base::Value> parameters_js;
for (const auto& entry : *parameters_) {
base::Value parameter_js = base::Value(base::Value::Type::DICTIONARY);
parameter_js.SetKey(entry.first, base::Value(entry.second));
parameters_js.push_back(std::move(parameter_js));
}
dict.SetKey("parameters", base::Value(parameters_js));
dict.SetKey("scripts", script_tracker_->GetDebugContext());
std::string output_js;
base::JSONWriter::Write(dict, &output_js);
return output_js;
}
bool Controller::AllowTouchEvent(float x, float y) { bool Controller::AllowTouchEvent(float x, float y) {
if (touchable_element_area_.IsEmpty()) { if (touchable_element_area_.IsEmpty()) {
// Allow all touch events if there is no touchable element area. // Allow all touch events if there is no touchable element area.
......
...@@ -85,6 +85,7 @@ class Controller : public ScriptExecutorDelegate, ...@@ -85,6 +85,7 @@ class Controller : public ScriptExecutorDelegate,
void OnDestroy() override; void OnDestroy() override;
void OnGiveUp() override; void OnGiveUp() override;
void OnScriptSelected(const std::string& script_path) override; void OnScriptSelected(const std::string& script_path) override;
std::string GetDebugContext() override;
bool AllowTouchEvent(float x, float y) override; bool AllowTouchEvent(float x, float y) override;
// Overrides ScriptTracker::Listener: // Overrides ScriptTracker::Listener:
......
...@@ -53,6 +53,7 @@ class MockUiController : public UiController { ...@@ -53,6 +53,7 @@ class MockUiController : public UiController {
MOCK_METHOD1(ShowDetails, void(const DetailsProto& details)); MOCK_METHOD1(ShowDetails, void(const DetailsProto& details));
MOCK_METHOD2(ShowProgressBar, void(int progress, const std::string& message)); MOCK_METHOD2(ShowProgressBar, void(int progress, const std::string& message));
MOCK_METHOD0(HideProgressBar, void()); MOCK_METHOD0(HideProgressBar, void());
MOCK_CONST_METHOD0(GetDebugContext, std::string());
}; };
} // namespace autofill_assistant } // namespace autofill_assistant
......
...@@ -7,6 +7,7 @@ ...@@ -7,6 +7,7 @@
#include <algorithm> #include <algorithm>
#include <utility> #include <utility>
#include "base/base64.h"
#include "base/bind.h" #include "base/bind.h"
#include "components/autofill_assistant/browser/script.h" #include "components/autofill_assistant/browser/script.h"
#include "components/autofill_assistant/browser/script_executor.h" #include "components/autofill_assistant/browser/script_executor.h"
...@@ -93,6 +94,40 @@ void ScriptTracker::ClearRunnableScripts() { ...@@ -93,6 +94,40 @@ void ScriptTracker::ClearRunnableScripts() {
runnable_scripts_.clear(); runnable_scripts_.clear();
} }
base::Value ScriptTracker::GetDebugContext() const {
base::Value dict(base::Value::Type::DICTIONARY);
std::string last_server_payload_js = last_server_payload_;
base::Base64Encode(last_server_payload_js, &last_server_payload_js);
dict.SetKey("last-payload", base::Value(last_server_payload_js));
std::vector<base::Value> executed_scripts_js;
for (const auto& entry : executed_scripts_) {
base::Value script_js = base::Value(base::Value::Type::DICTIONARY);
script_js.SetKey(entry.first, base::Value(entry.second));
executed_scripts_js.push_back(std::move(script_js));
}
dict.SetKey("executed-scripts", base::Value(executed_scripts_js));
std::vector<base::Value> available_scripts_js;
for (const auto& entry : available_scripts_)
available_scripts_js.push_back(base::Value(entry.second->handle.path));
dict.SetKey("available-scripts", base::Value(available_scripts_js));
std::vector<base::Value> runnable_scripts_js;
for (const auto& entry : runnable_scripts_) {
base::Value script_js = base::Value(base::Value::Type::DICTIONARY);
script_js.SetKey("name", base::Value(entry.name));
script_js.SetKey("path", base::Value(entry.path));
script_js.SetKey("initial_prompt", base::Value(entry.initial_prompt));
script_js.SetKey("autostart", base::Value(entry.autostart));
script_js.SetKey("highlight", base::Value(entry.highlight));
runnable_scripts_js.push_back(std::move(script_js));
}
dict.SetKey("runnable-scripts", base::Value(runnable_scripts_js));
return dict;
}
void ScriptTracker::OnScriptRun( void ScriptTracker::OnScriptRun(
const std::string& script_path, const std::string& script_path,
ScriptExecutor::RunScriptCallback original_callback, ScriptExecutor::RunScriptCallback original_callback,
......
...@@ -14,6 +14,7 @@ ...@@ -14,6 +14,7 @@
#include "base/macros.h" #include "base/macros.h"
#include "base/memory/weak_ptr.h" #include "base/memory/weak_ptr.h"
#include "base/time/time.h" #include "base/time/time.h"
#include "base/values.h"
#include "components/autofill_assistant/browser/script.h" #include "components/autofill_assistant/browser/script.h"
#include "components/autofill_assistant/browser/script_executor.h" #include "components/autofill_assistant/browser/script_executor.h"
#include "components/autofill_assistant/browser/service.pb.h" #include "components/autofill_assistant/browser/service.pb.h"
...@@ -84,6 +85,11 @@ class ScriptTracker : public ScriptExecutor::Listener { ...@@ -84,6 +85,11 @@ class ScriptTracker : public ScriptExecutor::Listener {
// script running at a time. // script running at a time.
bool running() const { return executor_ != nullptr; } bool running() const { return executor_ != nullptr; }
// Returns a dictionary describing the current execution context, which
// is intended to be serialized as JSON string. The execution context is
// useful when analyzing feedback forms and for debugging in general.
base::Value GetDebugContext() const;
private: private:
typedef std::map<Script*, std::unique_ptr<Script>> AvailableScriptMap; typedef std::map<Script*, std::unique_ptr<Script>> AvailableScriptMap;
......
...@@ -85,6 +85,10 @@ class UiController { ...@@ -85,6 +85,10 @@ class UiController {
// Hide the progress bar. // Hide the progress bar.
virtual void HideProgressBar() = 0; virtual void HideProgressBar() = 0;
// Returns a string describing the current execution context. This is useful
// when analyzing feedback forms and for debugging in general.
virtual std::string GetDebugContext() const = 0;
protected: protected:
UiController() = default; UiController() = default;
}; };
......
...@@ -31,6 +31,10 @@ class UiDelegate { ...@@ -31,6 +31,10 @@ class UiDelegate {
// Called when a script was selected for execution. // Called when a script was selected for execution.
virtual void OnScriptSelected(const std::string& script_path) = 0; virtual void OnScriptSelected(const std::string& script_path) = 0;
// Returns a string describing the current execution context. This is useful
// when analyzing feedback forms and for debugging in general.
virtual std::string GetDebugContext() = 0;
// Checks whether a touch event at the given position should be allowed. // Checks whether a touch event at the given position should be allowed.
// //
// Coordinates are values between 0 and 1 relative to the size of the visible // Coordinates are values between 0 and 1 relative to the size of the visible
......
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