[Vertical Tabs] Add the new badge to empty space context menu entry point in tablets [chromium/src : main]
Eleanor Lee (Gerrit)
Eleanor Lee added 1 comment![]( "Open in Gerrit")
Will add unit tests after the first round of reviews.
Related details
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gazal Agarwal (Gerrit)
Gazal Agarwal added 5 comments![]( "Open in Gerrit")
.withTitleRes(layoutTitleRes)This was setting the TITLE_ID property, and not the TITLE. Can you double check that won't cause any issues. You may need to update tests.
item.model.set(ListMenuItemProperties.TITLE, title);We can add a `withTitle(CharSequence)` method to ListItemBuilder.
/** Stores whether the user has clicked the "Show tabs vertically" menu item. */
public static final String VERTICAL_TABS_LAYOUT_TOGGLE_CLICKED =
"Chrome.VerticalTabs.LayoutToggleClicked";Could we just set the value of `VERTICAL_TABS_LAYOUT_TOGGLE_VIEW_COUNT` to max impression count, when user clicks on the menu item, and get rid of this?
public static boolean shouldShowNewBadgeForVerticalTabs(Context context) {nit: move this below `isGroupHeaderDragEnabled` to group with other public methods.
final SharedPreferencesManager sharedPreferences = ChromeSharedPreferences.getInstance();Nit: The final modifier on local variable sharedPreferences is unnecessary.
Related details
- Eleanor Lee
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Eleanor Lee (Gerrit)
Eleanor Lee added 7 comments![]( "Open in Gerrit")
Will add unit tests after the first round of reviews.
Done
Are we not allowed to use `DeviceInfo.isDesktop()` ?
.withTitleRes(layoutTitleRes)This was setting the TITLE_ID property, and not the TITLE. Can you double check that won't cause any issues. You may need to update tests.
Done
We can add a `withTitle(CharSequence)` method to ListItemBuilder.
Done
/** Stores whether the user has clicked the "Show tabs vertically" menu item. */
public static final String VERTICAL_TABS_LAYOUT_TOGGLE_CLICKED =
"Chrome.VerticalTabs.LayoutToggleClicked";Could we just set the value of `VERTICAL_TABS_LAYOUT_TOGGLE_VIEW_COUNT` to max impression count, when user clicks on the menu item, and get rid of this?
Done
public static boolean shouldShowNewBadgeForVerticalTabs(Context context) {nit: move this below `isGroupHeaderDragEnabled` to group with other public methods.
Done
final SharedPreferencesManager sharedPreferences = ChromeSharedPreferences.getInstance();Nit: The final modifier on local variable sharedPreferences is unnecessary.
- Gazal Agarwal
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |