Commit e20737d1 authored by Theresa's avatar Theresa Committed by Commit Bot

[Modern] Modernize the NTP & finish modernizing top toolbar

 * "Modernizes" ntp - change bg color, tweak values for fake search box
   to real omnibox transition, change fake search box bg, update tablet
   style
 * Increases modern toolbar background height to 40dp
 * Fixes an omnibox suggestions alignment issue in RTL to better align
   with omibox actions container
 * Updates toolbar shadow for NTP scroll
 * Moves toolbar primary color initialization to ToolbarModelImpl
   constructor

BUG=803087,803088

Change-Id: Ic656b5de656296c3fe592e5a0a3e106472fa0419
Reviewed-on: https://chromium-review.googlesource.com/902966Reviewed-by: default avatarTed Choc <tedchoc@chromium.org>
Reviewed-by: default avatarMatthew Jones <mdjones@chromium.org>
Commit-Queue: Theresa <twellington@chromium.org>
Cr-Commit-Position: refs/heads/master@{#534808}
parent d8fedb79
......@@ -274,8 +274,8 @@
<dimen name="bottom_location_bar_content_vertical_inset">4dp</dimen>
<!-- Modern toolbar background dimensions -->
<dimen name="modern_toolbar_background_size">36dp</dimen>
<dimen name="modern_toolbar_background_corner_radius">18dp</dimen>
<dimen name="modern_toolbar_background_size">40dp</dimen>
<dimen name="modern_toolbar_background_corner_radius">20dp</dimen>
<dimen name="modern_toolbar_background_lateral_inset">2dp</dimen>
<!-- Omnibox suggestions -->
......@@ -338,6 +338,11 @@
<dimen name="ntp_search_box_shadow_width">4dp</dimen>
<dimen name="ntp_search_box_transition_length">16dp</dimen>
<dimen name="ntp_search_box_logo_padding">8dp</dimen>
<dimen name="ntp_search_box_voice_search_margin_end_modern">6dp</dimen>
<!-- Lateral offset added to the search box bounds when modern is enabled. 8dp is added to both
the top and bottom bounds to bring the 40dp modern_toolbar_background_size to 56dp
(matches toolbar_height_no_shadow). -->
<dimen name="ntp_search_box_bounds_lateral_offset_modern">8dp</dimen>
<dimen name="ntp_wide_card_lateral_margins">48dp</dimen>
<dimen name="ntp_list_item_min_height">48dp</dimen>
<dimen name="ntp_shadow_height">9dp</dimen>
......
......@@ -878,12 +878,6 @@ public class ChromeTabbedActivity
mLayoutManager, mLayoutManager, tabSwitcherClickHandler, newTabClickHandler,
bookmarkClickHandler, null);
// TODO(twellington): Move to toolbar manager construction after isModernUiEnabled is
// available before native is loaded.
if (FeatureUtilities.isChromeModernDesignEnabled()) {
getToolbarManager().setUseModernDesign(true);
}
if (isTablet()) {
EmptyBackgroundViewWrapper bgViewWrapper = new EmptyBackgroundViewWrapper(
getTabModelSelector(), getTabCreator(false), ChromeTabbedActivity.this,
......
......@@ -34,6 +34,7 @@ import org.chromium.chrome.browser.profiles.Profile;
import org.chromium.chrome.browser.search_engines.TemplateUrlService;
import org.chromium.chrome.browser.search_engines.TemplateUrlService.TemplateUrlServiceObserver;
import org.chromium.chrome.browser.snackbar.SnackbarManager;
import org.chromium.chrome.browser.suggestions.SuggestionsConfig;
import org.chromium.chrome.browser.suggestions.SuggestionsDependencyFactory;
import org.chromium.chrome.browser.suggestions.SuggestionsEventReporter;
import org.chromium.chrome.browser.suggestions.SuggestionsMetrics;
......@@ -290,7 +291,9 @@ public class NewTabPage
mTileGroupDelegate = new NewTabPageTileGroupDelegate(activity, profile, navigationDelegate);
mTitle = activity.getResources().getString(R.string.button_new_tab);
mBackgroundColor = ApiCompatibilityUtils.getColor(activity.getResources(), R.color.ntp_bg);
mBackgroundColor = ApiCompatibilityUtils.getColor(activity.getResources(),
SuggestionsConfig.useModernLayout() ? R.color.modern_primary_color
: R.color.ntp_bg);
mThemeColor = ApiCompatibilityUtils.getColor(
activity.getResources(), R.color.default_primary_color);
TemplateUrlService.getInstance().addObserver(this);
......
......@@ -11,6 +11,7 @@ import android.content.Context;
import android.graphics.Canvas;
import android.graphics.Point;
import android.graphics.Rect;
import android.graphics.drawable.GradientDrawable;
import android.support.annotation.Nullable;
import android.support.v7.widget.DefaultItemAnimator;
import android.support.v7.widget.RecyclerView;
......@@ -146,6 +147,12 @@ public class NewTabPageView
private int mSnapshotScrollY;
private ContextMenuManager mContextMenuManager;
/**
* Lateral offset to add to the top and bottom of the search box bounds. May be 0 if no offset
* should be applied.
*/
private int mSearchBoxBoundsLateralOffset;
/**
* Manages the view interaction with the rest of the system.
*/
......@@ -279,7 +286,18 @@ public class NewTabPageView
mSearchBoxView = mNewTabPageLayout.findViewById(R.id.search_box);
if (SuggestionsConfig.useModernLayout()) {
ViewUtils.setNinePatchBackgroundResource(mSearchBoxView, R.drawable.card_modern);
mSearchBoxView.setBackgroundResource(R.drawable.modern_toolbar_background);
if (!DeviceFormFactor.isTablet()) {
mSearchBoxView.getLayoutParams().height = getResources().getDimensionPixelSize(
R.dimen.modern_toolbar_background_size);
mSearchBoxBoundsLateralOffset = getResources().getDimensionPixelSize(
R.dimen.ntp_search_box_bounds_lateral_offset_modern);
} else {
mSearchBoxView.getLayoutParams().height =
getResources().getDimensionPixelSize(R.dimen.toolbar_height_no_shadow);
GradientDrawable background = (GradientDrawable) mSearchBoxView.getBackground();
background.setCornerRadius(mSearchBoxView.getLayoutParams().height / 2.f);
}
}
mNoSearchLogoSpacer = mNewTabPageLayout.findViewById(R.id.no_search_logo_spacer);
......@@ -369,7 +387,7 @@ public class NewTabPageView
final TextView searchBoxTextView =
(TextView) mSearchBoxView.findViewById(R.id.search_box_text);
String hintText = getResources().getString(R.string.search_or_type_web_address);
if (!DeviceFormFactor.isTablet()) {
if (!DeviceFormFactor.isTablet() || SuggestionsConfig.useModernLayout()) {
searchBoxTextView.setHint(hintText);
} else {
searchBoxTextView.setContentDescription(hintText);
......@@ -430,6 +448,14 @@ public class NewTabPageView
mManager.focusSearchBox(true, null);
}
});
if (SuggestionsConfig.useModernLayout() && !DeviceFormFactor.isTablet()) {
ApiCompatibilityUtils.setMarginEnd(
(MarginLayoutParams) mVoiceSearchButton.getLayoutParams(),
getResources().getDimensionPixelSize(
R.dimen.ntp_search_box_voice_search_margin_end_modern));
}
TraceEvent.end(TAG + ".initializeVoiceSearchButton()");
}
......@@ -744,7 +770,8 @@ public class NewTabPageView
int basePosition = mRecyclerView.computeVerticalScrollOffset()
+ mNewTabPageLayout.getPaddingTop();
int target = Math.max(basePosition,
mSearchBoxView.getBottom() - mSearchBoxView.getPaddingBottom());
mSearchBoxView.getBottom() - mSearchBoxView.getPaddingBottom()
+ mSearchBoxBoundsLateralOffset);
mNewTabPageLayout.setTranslationY(percent * (basePosition - target));
}
......@@ -780,11 +807,11 @@ public class NewTabPageView
void getSearchBoxBounds(Rect bounds, Point translation) {
int searchBoxX = (int) mSearchBoxView.getX();
int searchBoxY = (int) mSearchBoxView.getY();
bounds.set(searchBoxX + mSearchBoxView.getPaddingLeft(),
searchBoxY + mSearchBoxView.getPaddingTop(),
searchBoxY + mSearchBoxView.getPaddingTop() - mSearchBoxBoundsLateralOffset,
searchBoxX + mSearchBoxView.getWidth() - mSearchBoxView.getPaddingRight(),
searchBoxY + mSearchBoxView.getHeight() - mSearchBoxView.getPaddingBottom());
searchBoxY + mSearchBoxView.getHeight() - mSearchBoxView.getPaddingBottom()
+ mSearchBoxBoundsLateralOffset);
translation.set(0, 0);
......
......@@ -91,8 +91,8 @@ class SuggestionView extends ViewGroup {
private OmniboxSuggestionDelegate mSuggestionDelegate;
private Boolean mUseDarkColors;
private int mPosition;
private int mRightOffsetPx;
private int mSuggestionViewOffset;
private int mRefineViewOffsetPx;
private int mSuggestionViewStartOffset;
private final SuggestionContentsContainer mContentsView;
......@@ -225,7 +225,8 @@ class SuggestionView extends ViewGroup {
contentsViewOffsetX + mContentsView.getMeasuredWidth(),
mContentsView.getMeasuredHeight());
int refineViewOffsetX = isRtl ? 0 : getMeasuredWidth() - mRefineWidth - mRightOffsetPx;
int refineViewOffsetX = isRtl ? mRefineViewOffsetPx
: (getMeasuredWidth() - mRefineWidth) - mRefineViewOffsetPx;
mRefineView.layout(
refineViewOffsetX,
0,
......@@ -321,8 +322,8 @@ class SuggestionView extends ViewGroup {
mContentsView.mTextLine2.setTextSize(TypedValue.COMPLEX_UNIT_PX, getResources()
.getDimension(R.dimen.omnibox_suggestion_second_line_text_size));
mRightOffsetPx = useModernDesign ? mRefineViewModernEndPadding : 0;
mSuggestionViewOffset = useModernDesign ? mSuggestionListModernOffset : 0;
mRefineViewOffsetPx = useModernDesign ? mRefineViewModernEndPadding : 0;
mSuggestionViewStartOffset = useModernDesign ? mSuggestionListModernOffset : 0;
// Suggestions with attached answers are rendered with rich results regardless of which
// suggestion type they are.
......@@ -875,18 +876,20 @@ class SuggestionView extends ViewGroup {
imageSpacing = getResources().getDimensionPixelOffset(
R.dimen.omnibox_suggestion_answer_image_horizontal_spacing);
}
if (isRTL) {
mTextLine1.layout(0, t, mTextRight - mSuggestionViewOffset, b);
mTextLine1.layout(0, t, mTextRight - mSuggestionViewStartOffset, b);
mAnswerImage.layout(
mTextRight - imageWidth, t, mTextRight - mSuggestionViewOffset, b);
mTextLine2.layout(
0, t, mTextRight - (imageWidth + imageSpacing) - mSuggestionViewOffset, b);
mTextRight - imageWidth, t, mTextRight - mSuggestionViewStartOffset, b);
mTextLine2.layout(0, t,
mTextRight - (imageWidth + imageSpacing) - mSuggestionViewStartOffset, b);
} else {
mTextLine1.layout(mTextLeft + mSuggestionViewOffset, t, r - l, b);
mTextLine1.layout(mTextLeft + mSuggestionViewStartOffset, t, r - l, b);
mAnswerImage.layout(
mTextLeft + mSuggestionViewOffset, t, mTextLeft + imageWidth, b);
mTextLeft + mSuggestionViewStartOffset, t, mTextLeft + imageWidth, b);
mTextLine2.layout(
mTextLeft + imageWidth + imageSpacing + mSuggestionViewOffset, t, r - l, b);
mTextLeft + imageWidth + imageSpacing + mSuggestionViewStartOffset, t,
r - l, b);
}
int suggestionIconPosition = getSuggestionIconLeftPosition();
......
......@@ -78,8 +78,11 @@ public final class SuggestionsConfig {
PARAM_CONDENSED_TILE_LAYOUT_FOR_LARGE_SCREENS_ENABLED, false);
}
/**
* @return Whether the modern layout should be used for suggestions.
*/
public static boolean useModernLayout() {
return FeatureUtilities.isChromeHomeEnabled()
return FeatureUtilities.isChromeModernDesignEnabled()
|| ChromeFeatureList.isEnabled(ChromeFeatureList.NTP_MODERN_LAYOUT);
}
}
......@@ -198,7 +198,8 @@ public class ToolbarManager implements ToolbarTabController, UrlFocusChangeListe
mActionBarDelegate = new ViewShiftingActionBarDelegate(activity, controlContainer);
}
mToolbarModel = new ToolbarModelImpl(activity.getBottomSheet());
mToolbarModel = new ToolbarModelImpl(activity.getBottomSheet(),
activity.supportsModernDesign() && FeatureUtilities.isChromeModernDesignEnabled());
mControlContainer = controlContainer;
assert mControlContainer != null;
......@@ -704,14 +705,6 @@ public class ToolbarManager implements ToolbarTabController, UrlFocusChangeListe
mInitializedWithNative = true;
}
/**
* @param useModernDesign Whether the modern design should be used for the toolbar managed by
* this manager.
*/
public void setUseModernDesign(boolean useModernDesign) {
mToolbarModel.setUseModernDesign(useModernDesign);
}
/**
* @return The bookmarks bridge.
*/
......
......@@ -10,7 +10,6 @@ import android.support.annotation.DrawableRes;
import android.support.annotation.Nullable;
import android.text.TextUtils;
import org.chromium.base.ApiCompatibilityUtils;
import org.chromium.base.ContextUtils;
import org.chromium.base.VisibleForTesting;
import org.chromium.chrome.R;
......@@ -38,21 +37,24 @@ import org.chromium.ui.base.DeviceFormFactor;
*/
class ToolbarModelImpl extends ToolbarModel implements ToolbarDataProvider, ToolbarModelDelegate {
private final BottomSheet mBottomSheet;
private final boolean mUseModernDesign;
private Tab mTab;
private boolean mIsIncognito;
private int mPrimaryColor;
private boolean mIsUsingBrandColor;
private boolean mUseModernDesign;
/**
* Default constructor for this class.
* @param bottomSheet The {@link BottomSheet} for the activity displaying this toolbar.
* @param useModernDesign Whether the modern design should be used for the toolbar represented
* by this model.
*/
public ToolbarModelImpl(@Nullable BottomSheet bottomSheet) {
public ToolbarModelImpl(@Nullable BottomSheet bottomSheet, boolean useModernDesign) {
super();
mBottomSheet = bottomSheet;
mPrimaryColor = ApiCompatibilityUtils.getColor(
ContextUtils.getApplicationContext().getResources(),
R.color.default_primary_color);
mUseModernDesign = useModernDesign;
mPrimaryColor = ColorUtils.getDefaultThemeColor(
ContextUtils.getApplicationContext().getResources(), useModernDesign, false);
}
/**
......@@ -62,16 +64,6 @@ class ToolbarModelImpl extends ToolbarModel implements ToolbarDataProvider, Tool
initialize(this);
}
/**
* @param useModernDesign Whether the modern design should be used for the toolbar represented
* by this model.
*/
public void setUseModernDesign(boolean useModernDesign) {
mUseModernDesign = useModernDesign;
setPrimaryColor(ColorUtils.getDefaultThemeColor(
ContextUtils.getApplicationContext().getResources(), useModernDesign, false));
}
@Override
public WebContents getActiveWebContents() {
if (!hasTab()) return null;
......
......@@ -279,10 +279,10 @@ public class ToolbarPhone extends ToolbarLayout
private float mModernLocationBarContentLateralInset;
/**
* The extra margin to apply to the left side of the location bar when it is focused and the
* The extra margin to apply to the start side of the location bar when it is focused and the
* modern UI is enabled.
*/
private int mModernLocationBarExtraFocusedLeftMargin;
private int mModernLocationBarExtraFocusedStartMargin;
/**
* Used to specify the visual state of the toolbar.
......@@ -400,8 +400,9 @@ public class ToolbarPhone extends ToolbarLayout
R.dimen.bottom_location_bar_content_lateral_inset);
mModernLocationBarContentVerticalInset = getResources().getDimensionPixelSize(
R.dimen.bottom_location_bar_content_vertical_inset);
mModernLocationBarExtraFocusedLeftMargin = getResources().getDimensionPixelSize(
mModernLocationBarExtraFocusedStartMargin = getResources().getDimensionPixelSize(
R.dimen.bottom_toolbar_background_focused_left_margin);
mLocationBarBackgroundCornerRadius = 0;
} else {
mLocationBarVerticalMargin =
getResources().getDimensionPixelOffset(R.dimen.location_bar_vertical_margin);
......@@ -481,14 +482,6 @@ public class ToolbarPhone extends ToolbarLayout
public void onNativeLibraryReady() {
super.onNativeLibraryReady();
// TODO(twellington): Move this to constructor when isModernUiEnabled() is available before
// native is loaded.
if (mLocationBar.useModernDesign()) {
mNewTabButton.setIsModern();
if (mToolbarShadow != null) mToolbarShadow.setImageDrawable(getToolbarShadowDrawable());
initLocationBarBackground();
}
getLocationBar().onNativeLibraryReady();
enableTabSwitchingResources();
......@@ -513,6 +506,8 @@ public class ToolbarPhone extends ToolbarLayout
});
onHomeButtonUpdate(HomepageManager.isHomepageEnabled());
if (mLocationBar.useModernDesign()) mNewTabButton.setIsModern();
updateVisualsForToolbarState();
}
......@@ -723,7 +718,7 @@ public class ToolbarPhone extends ToolbarLayout
int width = containerWidth - (2 * mToolbarSidePadding) + priorVisibleWidth;
if (mLocationBar.useModernDesign()) {
width = width - mModernLocationBarExtraFocusedLeftMargin
width = width - mModernLocationBarExtraFocusedStartMargin
- mLocationBarBackgroundPadding.left - mLocationBarBackgroundPadding.right;
}
......@@ -736,11 +731,11 @@ public class ToolbarPhone extends ToolbarLayout
*/
protected int getFocusedLocationBarLeftMargin(int priorVisibleWidth) {
if (mLocationBar.useModernDesign()) {
int baseMargin = mToolbarSidePadding + mModernLocationBarExtraFocusedLeftMargin;
int baseMargin = mToolbarSidePadding + mLocationBarBackgroundPadding.left;
if (ApiCompatibilityUtils.isLayoutRtl(mLocationBar)) {
return baseMargin - mLocationBarBackgroundPadding.right;
return baseMargin;
} else {
return baseMargin - priorVisibleWidth + mLocationBarBackgroundPadding.left;
return baseMargin - priorVisibleWidth + mModernLocationBarExtraFocusedStartMargin;
}
}
......@@ -760,7 +755,7 @@ public class ToolbarPhone extends ToolbarLayout
// Uses getMeasuredWidth()s instead of getLeft() because this is called in onMeasure
// and the layout values have not yet been set.
if (visualState == VisualState.NEW_TAB_NORMAL) {
return 0;
return mLocationBar.useModernDesign() ? mToolbarSidePadding : 0;
} else if (ApiCompatibilityUtils.isLayoutRtl(this)) {
return getBoundsAfterAccountingForRightButtons();
} else {
......@@ -788,7 +783,7 @@ public class ToolbarPhone extends ToolbarLayout
// Uses getMeasuredWidth()s instead of getRight() because this is called in onMeasure
// and the layout values have not yet been set.
if (visualState == VisualState.NEW_TAB_NORMAL) {
return getMeasuredWidth();
return getMeasuredWidth() - (mLocationBar.useModernDesign() ? mToolbarSidePadding : 0);
} else if (ApiCompatibilityUtils.isLayoutRtl(this)) {
return getMeasuredWidth() - getBoundsAfterAccountingForLeftButton();
} else {
......@@ -816,6 +811,13 @@ public class ToolbarPhone extends ToolbarLayout
Resources res = getResources();
switch (visualState) {
case NEW_TAB_NORMAL:
if (mLocationBar.useModernDesign() && mUrlExpansionPercent == 1.f) {
// When the location bar reaches the top of the screen, the background needs
// to change back to the default, solid color so that the NTP content is
// not visible beneath the toolbar.
return ColorUtils.getDefaultThemeColor(
getResources(), mLocationBar.useModernDesign(), false);
}
return Color.TRANSPARENT;
case NORMAL:
return ColorUtils.getDefaultThemeColor(
......@@ -1038,6 +1040,7 @@ public class ToolbarPhone extends ToolbarLayout
if (isLocationBarRtl) {
locationBarBaseTranslationX += mUnfocusedLocationBarLayoutWidth - currentWidth;
}
locationBarBaseTranslationX *= 1f - mUrlExpansionPercent;
mLocationBarBackgroundNtpOffset.setEmpty();
......@@ -1122,10 +1125,13 @@ public class ToolbarPhone extends ToolbarLayout
mToolbarButtonsContainer.setTranslationY(0);
if (mHomeButton != null) mHomeButton.setTranslationY(0);
}
if (!mToolbarShadowPermanentlyHidden) {
if (!mToolbarShadowPermanentlyHidden
&& !(mLocationBar.useModernDesign() && mUrlFocusChangeInProgress)) {
mToolbarShadow.setAlpha(
mLocationBar.useModernDesign() && mUrlBar.hasFocus() ? 0.f : 1.f);
}
mLocationBar.setAlpha(1);
mForceDrawLocationBarBackground = false;
mLocationBarBackgroundAlpha = 255;
......@@ -1134,6 +1140,7 @@ public class ToolbarPhone extends ToolbarLayout
&& !mLocationBar.hasFocus())) {
mLocationBarBackgroundAlpha = LOCATION_BAR_TRANSPARENT_BACKGROUND_ALPHA;
}
setAncestorsShouldClipChildren(true);
mNtpSearchBoxScrollPercent = UNINITIALIZED_PERCENT;
updateUrlExpansionPercent();
......@@ -1148,13 +1155,22 @@ public class ToolbarPhone extends ToolbarLayout
if (mTabSwitcherState == TAB_SWITCHER || mTabSwitcherState == ENTERING_TAB_SWITCHER) return;
setAncestorsShouldClipChildren(mUrlExpansionPercent == 0f);
if (!mToolbarShadowPermanentlyHidden) mToolbarShadow.setAlpha(0f);
if (!mToolbarShadowPermanentlyHidden
&& !(mLocationBar.useModernDesign() && mUrlFocusChangeInProgress)) {
float alpha = 0.f;
if (mLocationBar.useModernDesign() && !mUrlBar.hasFocus()
&& mNtpSearchBoxScrollPercent == 1.f) {
alpha = 1.f;
}
mToolbarShadow.setAlpha(alpha);
}
NewTabPage ntp = getToolbarDataProvider().getNewTabPageForCurrentTab();
ntp.getSearchBoxBounds(mNtpSearchBoxBounds, mNtpSearchBoxTranslation);
int locationBarTranslationY =
Math.max(0, (mNtpSearchBoxBounds.top - mLocationBar.getTop()));
mLocationBar.setTranslationY(locationBarTranslationY);
updateButtonsTranslationY();
// Linearly interpolate between the bounds of the search box on the NTP and the omnibox
......@@ -1186,6 +1202,8 @@ public class ToolbarPhone extends ToolbarLayout
// The search box on the NTP is visible if our omnibox is invisible, and vice-versa.
ntp.setSearchBoxAlpha(1f - relativeAlpha);
updateToolbarBackground(mVisualState);
}
/**
......@@ -1492,7 +1510,10 @@ public class ToolbarPhone extends ToolbarLayout
// This is a workaround for http://crbug.com/574928. Since Jelly Bean is the lowest version
// we support now and the next deprecation target, we decided to simply workaround.
if (Build.VERSION.SDK_INT <= Build.VERSION_CODES.JELLY_BEAN) {
// The drawable also has to be set here when using modern design.
// TODO(twellington): Update XML for modern and remove || check after modern launches.
if (Build.VERSION.SDK_INT <= Build.VERSION_CODES.JELLY_BEAN
|| mLocationBar.useModernDesign()) {
mToolbarShadow.setImageDrawable(getToolbarShadowDrawable());
}
}
......@@ -2039,7 +2060,7 @@ public class ToolbarPhone extends ToolbarLayout
}
}
if (mLocationBar.useModernDesign() && !isLocationBarShownInNTP()) {
if (mLocationBar.useModernDesign()) {
animator = ObjectAnimator.ofFloat(mToolbarShadow, ALPHA, 1);
animator.setDuration(URL_FOCUS_CHANGE_ANIMATION_DURATION_MS);
animator.setInterpolator(BakedBezierInterpolator.TRANSFORM_CURVE);
......
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