[BrowserControls] Allow adjustment tab strip height during startup [chromium/src : main]
Wenyu Fu (Gerrit)
Wenyu Fu voted and added 1 comment![]( "Open in Gerrit")
Votes added by Wenyu Fu
| Commit-Queue | +1 |
1 comment
PTAL
I gate this fix using the LockTopControlsV2 flag; it's technically more appropriate to use the TopControlsRefactorV2, but the risk is that feature is bundled with bookmark bar, and we'll have to make edits to the bookmark bar experiment config if this becomes broken.
Let me know your thoughts.
Related details
- Aishwarya Rajesh
- Mark Schillaci
- Sirisha Kavuluru
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Aishwarya Rajesh (Gerrit)
Aishwarya Rajesh added 6 comments![]( "Open in Gerrit")
// from TabStripTransitionCoordinator. When canForceTopChromeHeightAdjustmentOnStartup,
// TabStripTopControlLayer is already adjusted to the TSTC height at an earlier timing,```suggestion
// from TabStripTransitionCoordinator. When canForceTopChromeHeightAdjustmentOnStartup() returns true,
// TabStripTopControlLayer is already adjusted to this internal value at an earlier timing,
```
/**It is technically possible for this method to be called in a non-startup flow, or without a null check on the tab strip delegate, correct? The `*OnStartup` naming is good indication this is expected to be called during startup but maybe we should explicitly call this out in the method docs so callers are aware that this may not WAI if called from a non-startup context.
Although it seems like we will eventually get rid of this method post-launch and we have asserts in place to gauge if this method is called in a non-startup flow so maybe this is okay.
// Set up the coordinator. Launching Chrome in DW mode, and test the coordinator request
// went/
// through without waiting for resource adapter.format; suggestion: verify that the coordinator request goes through without waiting for the resource adapter.
// Set up the coordinator. Launching Chrome in DW mode, and test the coordinator requests/verify that the coordinator request does not go through.
// Set up the coordinator. Launching Chrome in DW mode, and test the coordinator request
setUpTabStripTransitionCoordinator(
false, LARGE_NORMAL_WINDOW_WIDTH, /* initDelegate= */ false);
Assert.assertEquals(
"Height request should not go through when feature disabled.",
NOTHING_OBSERVED,
mTestHandler.heightRequested);```suggestion
// Set up the coordinator. Launching Chrome in non-DW mode, verify that the coordinator request does not go through.
setUpTabStripTransitionCoordinator(
false, LARGE_NORMAL_WINDOW_WIDTH, /* initDelegate= */ false);
Assert.assertEquals(
"Height request should not go through when not in desktop windowing mode.",
NOTHING_OBSERVED,
mTestHandler.heightRequested);
```
Related details
- Mark Schillaci
- Sirisha Kavuluru
- Wenyu Fu
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. |
Aishwarya Rajesh (Gerrit)
Aishwarya Rajesh voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Related details
- Mark Schillaci
- Sirisha Kavuluru
- Wenyu Fu
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. |
Mark Schillaci (Gerrit)
Mark Schillaci added 1 comment![]( "Open in Gerrit")
LOCK_TOP_CONTROLS_ON_LARGE_TABLETS_V2, "adjust_tab_strip_on_startup", true);drive-by: would it help at all to include multiple variations in chrome://flags so QA can try testing with this param both false and true?
Related details
- Sirisha Kavuluru
- Wenyu Fu
Sirisha Kavuluru (Gerrit)
Sirisha Kavuluru added 4 comments![]( "Open in Gerrit")
topControlsStacker,
homeButtonDisplay,
mTabStripTopControlLayer);nit: Move mTabStripTopControlLayer closer to related topControlsStacker
if (!BrowserControlsUtils.canForceTopChromeHeightAdjustmentOnStartup()) {
mTabStripTopControlLayer.set(mToolbar.getTabStripHeight());
}Maybe tied to the other comment - post launch, if we pre-initialize TabStripTransitionCoordinator for all tablets, this will not be required here since TabStripTopControlLayer would be adjusted elsewhere. Is that right?
Wondering if instead of canForceTopChromeHeightAdjustmentOnStartup check, this should check whether TabStripTransitionCoordinator was pre-initialized vs not
private boolean canTransitionWithoutTabStrip() {I have minimal context on this class - what does transition without tab strip mean for this layer?
Is the transition here referring to TabStrip height transition? We want to kick this off even if tab strip is not drawn?
// When we can force height adjustment on start up, we need to create tab strip transition
// earlier, before native is ready.
if (BrowserControlsUtils.canForceTopChromeHeightAdjustmentOnStartup()) {Right now canForceTopChromeHeightAdjustmentOnStartup checks for large tablet along with flag checks. Post launch, would it be safe to initialize mTabStripTransitionCoordinator early on any tablet to avoid forking logic?
If so, can we add a TODO to capture this
Related details
- Wenyu Fu
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. |
Wenyu Fu (Gerrit)
Wenyu Fu added 9 comments![]( "Open in Gerrit")
// from TabStripTransitionCoordinator. When canForceTopChromeHeightAdjustmentOnStartup,
// TabStripTopControlLayer is already adjusted to the TSTC height at an earlier timing,```suggestion
// from TabStripTransitionCoordinator. When canForceTopChromeHeightAdjustmentOnStartup() returns true,
// TabStripTopControlLayer is already adjusted to this internal value at an earlier timing,
```
Done
if (!BrowserControlsUtils.canForceTopChromeHeightAdjustmentOnStartup()) {
mTabStripTopControlLayer.set(mToolbar.getTabStripHeight());
}Maybe tied to the other comment - post launch, if we pre-initialize TabStripTransitionCoordinator for all tablets, this will not be required here since TabStripTopControlLayer would be adjusted elsewhere. Is that right?
Wondering if instead of canForceTopChromeHeightAdjustmentOnStartup check, this should check whether TabStripTransitionCoordinator was pre-initialized vs not
My intention to keep this "flag" here is to ensure 1) in short term, it's clear the behavior difference based on the flag settings and 2) post launch, this call can be cleaned up.
Checking whether TSTC was pre-initialized might make the code feel clunky, as we need to keep a reference to it like
```
boolean mIsTstcInitialized;
ToolbarManager() {
// ctor...mIsTstcInitialized = mToolbar.getTstc() != null;
}
public initializeWithNative() {
// ...
if (mIsTstcInitialized) {
mTabStripTopControlLayer.set(mToolbar.getTabStripHeight());
}
// ...
}
```If you don't feel strongly about checking whether TSTC is initialized, my preference is keep the current style. Let me know :)
It is technically possible for this method to be called in a non-startup flow, or without a null check on the tab strip delegate, correct? The `*OnStartup` naming is good indication this is expected to be called during startup but maybe we should explicitly call this out in the method docs so callers are aware that this may not WAI if called from a non-startup context.
Although it seems like we will eventually get rid of this method post-launch and we have asserts in place to gauge if this method is called in a non-startup flow so maybe this is okay.
Thanks for the thoughtful comment. I made this method as a helper to check feature flags, so it's not meant to be only called during the start up flow.
Renamed this to `is*Enabled` to make that obvious.
LOCK_TOP_CONTROLS_ON_LARGE_TABLETS_V2, "adjust_tab_strip_on_startup", true);drive-by: would it help at all to include multiple variations in chrome://flags so QA can try testing with this param both false and true?
Thanks for the suggestion. Since it's a bug fix, it doesn't make sense to turn this off other than using it as a killswitch, so I did not include this.
private boolean canTransitionWithoutTabStrip() {I have minimal context on this class - what does transition without tab strip mean for this layer?
Is the transition here referring to TabStrip height transition? We want to kick this off even if tab strip is not drawn?
what does transition without tab strip mean for this layer
The "TabStrip" here refers to `mTabStrip`, which is the TabStripSceneLayerHolder implemented by StripLayoutHelperManager. The goal of this change is to start transition during Chrome startup, so it'll happen before native is ready, when StripLayoutHelperManager is initialized
Is the transition here referring to TabStrip height transition?
Yes - The "Transition" here refers to the tab strip transition, which is also how we adjust the app header height during Chrome startup.
We want to kick this off even if the tab strip is not drawn?
This is the key of this CL - tab strip transition involved adjustment not only for the StripLayoutHelperManager, it also needs adjustment for the ControlContainer to position the toolbar and update the tab strip placeholder drawable.
// When we can force height adjustment on start up, we need to create tab strip transition
// earlier, before native is ready.
if (BrowserControlsUtils.canForceTopChromeHeightAdjustmentOnStartup()) {Right now canForceTopChromeHeightAdjustmentOnStartup checks for large tablet along with flag checks. Post launch, would it be safe to initialize mTabStripTransitionCoordinator early on any tablet to avoid forking logic?
If so, can we add a TODO to capture this
Good point, done.
// Set up the coordinator. Launching Chrome in DW mode, and test the coordinator request
// went/
// through without waiting for resource adapter.format; suggestion: verify that the coordinator request goes through without waiting for the resource adapter.
Done
// Set up the coordinator. Launching Chrome in DW mode, and test the coordinator requests/verify that the coordinator request does not go through.
Done
// Set up the coordinator. Launching Chrome in DW mode, and test the coordinator request
setUpTabStripTransitionCoordinator(
false, LARGE_NORMAL_WINDOW_WIDTH, /* initDelegate= */ false);
Assert.assertEquals(
"Height request should not go through when feature disabled.",
NOTHING_OBSERVED,
mTestHandler.heightRequested);```suggestion
// Set up the coordinator. Launching Chrome in non-DW mode, verify that the coordinator request does not go through.
setUpTabStripTransitionCoordinator(
false, LARGE_NORMAL_WINDOW_WIDTH, /* initDelegate= */ false);
Assert.assertEquals(
"Height request should not go through when not in desktop windowing mode.",
NOTHING_OBSERVED,
mTestHandler.heightRequested);
```
- Mark Schillaci
- Sirisha Kavuluru
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. |
Wenyu Fu (Gerrit)
Wenyu Fu voted and added 1 comment![]( "Open in Gerrit")
Votes added by Wenyu Fu
| Commit-Queue | +1 |
1 comment
topControlsStacker,
homeButtonDisplay,
mTabStripTopControlLayer);nit: Move mTabStripTopControlLayer closer to related topControlsStacker
Done
Related details
- Mark Schillaci
- Sirisha Kavuluru
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. |
Sirisha Kavuluru (Gerrit)
Sirisha Kavuluru voted and added 2 comments![]( "Open in Gerrit")
Votes added by Sirisha Kavuluru
| Code-Review | +1 |
2 comments
if (!BrowserControlsUtils.canForceTopChromeHeightAdjustmentOnStartup()) {
mTabStripTopControlLayer.set(mToolbar.getTabStripHeight());
}Wenyu FuMaybe tied to the other comment - post launch, if we pre-initialize TabStripTransitionCoordinator for all tablets, this will not be required here since TabStripTopControlLayer would be adjusted elsewhere. Is that right?
Wondering if instead of canForceTopChromeHeightAdjustmentOnStartup check, this should check whether TabStripTransitionCoordinator was pre-initialized vs not
My intention to keep this "flag" here is to ensure 1) in short term, it's clear the behavior difference based on the flag settings and 2) post launch, this call can be cleaned up.
Checking whether TSTC was pre-initialized might make the code feel clunky, as we need to keep a reference to it like
```
boolean mIsTstcInitialized;ToolbarManager() {
// ctor...mIsTstcInitialized = mToolbar.getTstc() != null;
}public initializeWithNative() {
// ...
if (mIsTstcInitialized) {
mTabStripTopControlLayer.set(mToolbar.getTabStripHeight());
}
// ...
}
```If you don't feel strongly about checking whether TSTC is initialized, my preference is keep the current style. Let me know :)
If this can be cleanup post launch, can we add a TODO here, something like
" TODO: Once canForceTopChromeHeightAdjustmentOnStartup flags are launched, this setter can be removed for all form factors since TSTC will be early initialized for all tablets and this setter will be called elsewhere"
Without the TODO, this reads as we will need to keep this logic for non-large tablets.
private boolean canTransitionWithoutTabStrip() {Wenyu FuI have minimal context on this class - what does transition without tab strip mean for this layer?
Is the transition here referring to TabStrip height transition? We want to kick this off even if tab strip is not drawn?
what does transition without tab strip mean for this layer
The "TabStrip" here refers to `mTabStrip`, which is the TabStripSceneLayerHolder implemented by StripLayoutHelperManager. The goal of this change is to start transition during Chrome startup, so it'll happen before native is ready, when StripLayoutHelperManager is initialized
Is the transition here referring to TabStrip height transition?
Yes - The "Transition" here refers to the tab strip transition, which is also how we adjust the app header height during Chrome startup.
We want to kick this off even if the tab strip is not drawn?
This is the key of this CL - tab strip transition involved adjustment not only for the StripLayoutHelperManager, it also needs adjustment for the ControlContainer to position the toolbar and update the tab strip placeholder drawable.
- Mark Schillaci
- Wenyu Fu
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. |
Wenyu Fu (Gerrit)
Wenyu Fu added 1 comment![]( "Open in Gerrit")
if (!BrowserControlsUtils.canForceTopChromeHeightAdjustmentOnStartup()) {
mTabStripTopControlLayer.set(mToolbar.getTabStripHeight());
}Wenyu FuMaybe tied to the other comment - post launch, if we pre-initialize TabStripTransitionCoordinator for all tablets, this will not be required here since TabStripTopControlLayer would be adjusted elsewhere. Is that right?
Wondering if instead of canForceTopChromeHeightAdjustmentOnStartup check, this should check whether TabStripTransitionCoordinator was pre-initialized vs not
Sirisha KavuluruMy intention to keep this "flag" here is to ensure 1) in short term, it's clear the behavior difference based on the flag settings and 2) post launch, this call can be cleaned up.
Checking whether TSTC was pre-initialized might make the code feel clunky, as we need to keep a reference to it like
```
boolean mIsTstcInitialized;ToolbarManager() {
// ctor...mIsTstcInitialized = mToolbar.getTstc() != null;
}public initializeWithNative() {
// ...
if (mIsTstcInitialized) {
mTabStripTopControlLayer.set(mToolbar.getTabStripHeight());
}
// ...
}
```If you don't feel strongly about checking whether TSTC is initialized, my preference is keep the current style. Let me know :)
If this can be cleanup post launch, can we add a TODO here, something like
" TODO: Once canForceTopChromeHeightAdjustmentOnStartup flags are launched, this setter can be removed for all form factors since TSTC will be early initialized for all tablets and this setter will be called elsewhere"
Without the TODO, this reads as we will need to keep this logic for non-large tablets.
Done
Related details
- Mark Schillaci
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Wenyu Fu (Gerrit)
Wenyu Fu voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change with unreviewed changes![]( "Open in Gerrit")
Unreviewed changes
10 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarManager.java
Insertions: 2, Deletions: 1.
@@ -2456,7 +2456,8 @@
// from TabStripTransitionCoordinator. When canForceTopChromeHeightAdjustmentOnStartup()
// returns true, TabStripTopControlLayer is already adjusted to this internal height at an
// earlier timing, so this call can be skipped.
- if (!BrowserControlsUtils.isForceTopChromeHeightAdjustmentOnStartupEnabled()) {
+ // TODO(crbug.com/450970998): This call can be remove once feature launch.
+ if (!BrowserControlsUtils.isForceTopChromeHeightAdjustmentOnStartupEnabled(mActivity)) {
mTabStripTopControlLayer.set(mToolbar.getTabStripHeight());
}
```
```
The name of the file: chrome/browser/ui/android/toolbar/java/src/org/chromium/chrome/browser/toolbar/top/tab_strip/TabStripTransitionCoordinatorUnitTest.java
Insertions: 1, Deletions: 0.
@@ -1013,6 +1013,7 @@
ChromeFeatureList.LOCK_TOP_CONTROLS_ON_LARGE_TABLETS_V2
+ ":adjust_tab_strip_on_startup/true"
})
+ @Config(qualifiers = "sw720dp")
public void adjustOnStartup_OnDesktopWindowUpdate_Wide() {
// Deliberately having the resource adapter no-op to test startup flow.
doNothing().when(mViewResourceAdapter).triggerBitmapCapture();
```
```
The name of the file: chrome/browser/ui/android/tabstrip/java/src/org/chromium/chrome/browser/tabstrip/TabStripTopControlLayer.java
Insertions: 2, Deletions: 1.
@@ -339,6 +339,7 @@
}
private boolean canTransitionWithoutTabStrip() {
- return BrowserControlsUtils.isForceTopChromeHeightAdjustmentOnStartupEnabled();
+ return BrowserControlsUtils.isForceTopChromeHeightAdjustmentOnStartupEnabled(
+ mControlContainer.getView().getContext());
}
}
```
```
The name of the file: chrome/browser/ui/android/toolbar/java/src/org/chromium/chrome/browser/toolbar/top/tab_strip/HeightTransitionHandler.java
Insertions: 2, Deletions: 1.
@@ -542,7 +542,8 @@
/** Whether we can perform a tab strip transition right away, skip waiting on the delegate. */
private boolean canForceTransitionDuringStartup() {
- if (!BrowserControlsUtils.isForceTopChromeHeightAdjustmentOnStartupEnabled()) {
+ if (!BrowserControlsUtils.isForceTopChromeHeightAdjustmentOnStartupEnabled(
+ mControlContainer.getView().getContext())) {
return false;
}
```
```
The name of the file: chrome/browser/ui/android/tabstrip/java/src/org/chromium/chrome/browser/tabstrip/TabStripTopControlLayerUnitTest.java
Insertions: 7, Deletions: 6.
@@ -12,6 +12,8 @@
import static org.mockito.Mockito.times;
import static org.mockito.Mockito.verify;
+import android.view.View;
+
import org.junit.Before;
import org.junit.Test;
import org.junit.runner.RunWith;
@@ -20,11 +22,10 @@
import org.chromium.base.test.BaseRobolectricTestRunner;
import org.chromium.base.test.util.CallbackHelper;
-import org.chromium.base.test.util.Features.EnableFeatures;
import org.chromium.chrome.browser.browser_controls.BrowserControlsStateProvider;
+import org.chromium.chrome.browser.browser_controls.BrowserControlsUtils;
import org.chromium.chrome.browser.browser_controls.TopControlsStacker;
import org.chromium.chrome.browser.browser_controls.TopControlsStacker.TopControlType;
-import org.chromium.chrome.browser.flags.ChromeFeatureList;
import org.chromium.chrome.browser.toolbar.ControlContainer;
/** Unit tests for {@link TabStripTopControlLayer}. */
@@ -35,6 +36,7 @@
@Mock private ControlContainer mControlContainer;
@Mock private TabStripSceneLayerHolder mTabStripSceneLayerHolder;
@Mock private BrowserControlsStateProvider mBrowserControls;
+ @Mock private View mControlContainerView;
private TabStripTopControlLayer mTabStripTopControlLayer;
private final CallbackHelper mOnTransitionStartedCallback = new CallbackHelper();
@@ -43,6 +45,7 @@
public void setUp() {
MockitoAnnotations.openMocks(this);
doReturn(true).when(mTopControlsStacker).isLayerAtTop(TopControlType.TABSTRIP);
+ doReturn(mControlContainerView).when(mControlContainer).getView();
mTabStripTopControlLayer =
new TabStripTopControlLayer(
0, mTopControlsStacker, mBrowserControls, mControlContainer);
@@ -147,11 +150,9 @@
}
@Test
- @EnableFeatures({
- ChromeFeatureList.LOCK_TOP_CONTROLS_ON_LARGE_TABLETS_V2
- + ":adjust_tab_strip_up_startup/true"
- })
public void testTransitionWithoutTabStrip() {
+ BrowserControlsUtils.setsSyncMinHeightWithTotalHeightForTesting(true);
+
// Recreate the layer without setting the tab strip (without calling initializeWithNative).
mTabStripTopControlLayer =
new TabStripTopControlLayer(
```
```
The name of the file: chrome/browser/ui/android/toolbar/java/src/org/chromium/chrome/browser/toolbar/top/TopToolbarCoordinator.java
Insertions: 2, Deletions: 1.
@@ -304,7 +304,8 @@
// earlier, before native is ready.
// TODO(crbug.com/450970998): Once launched, it's safe to always call
// maybeInitializeTabStripTransitionCoordinator on all form factors.
- if (BrowserControlsUtils.isForceTopChromeHeightAdjustmentOnStartupEnabled()) {
+ if (BrowserControlsUtils.isForceTopChromeHeightAdjustmentOnStartupEnabled(
+ toolbarLayout.getContext())) {
mTabStripTransitionCoordinator =
maybeInitializeTabStripTransitionCoordinator(
mToolbarLayout,
```
```
The name of the file: chrome/browser/browser_controls/android/java/src/org/chromium/chrome/browser/browser_controls/BrowserControlsUtils.java
Insertions: 2, Deletions: 2.
@@ -46,13 +46,13 @@
}
/** Whether force adjusting top chrome height is allowed based on feature flags. */
- public static boolean isForceTopChromeHeightAdjustmentOnStartupEnabled() {
+ public static boolean isForceTopChromeHeightAdjustmentOnStartupEnabled(Context context) {
// Note: the check for feature doSyncMinHeightWithTotalHeightV2 is not necessary once the
// feature flag is launched. Once we are ready to cleanup the param
// sLockTopControlsForceAdjustHeightOnStartup it's safe to assume this method to return
// true always.
return isTopControlsRefactorOffsetEnabled()
- && doSyncMinHeightWithTotalHeightV2()
+ && doSyncMinHeightWithTotalHeightV2(context)
&& ChromeFeatureList.sLockTopControlsForceAdjustHeightOnStartup.getValue();
}
```
Change information
[BrowserControls] Allow adjustment tab strip height during startup
Part 2 of the complete POC: crrev.com/c/7427223
Create TabStripTransitionCoordinator early, before native is ready, so
we can just the toolbar's position and tab strip height immediately when
the app header insets are updated. This change reduced the frame jumps
when Chrome is launched in the desktop windowing mode.
- M chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarManager.java
- M chrome/browser/browser_controls/android/java/src/org/chromium/chrome/browser/browser_controls/BrowserControlsUtils.java
- M chrome/browser/flags/android/java/src/org/chromium/chrome/browser/flags/ChromeFeatureList.java
- M chrome/browser/ui/android/tabstrip/BUILD.gn
- M chrome/browser/ui/android/tabstrip/java/src/org/chromium/chrome/browser/tabstrip/TabStripTopControlLayer.java
- M chrome/browser/ui/android/tabstrip/java/src/org/chromium/chrome/browser/tabstrip/TabStripTopControlLayerUnitTest.java
- M chrome/browser/ui/android/toolbar/java/src/org/chromium/chrome/browser/toolbar/top/TopToolbarCoordinator.java
- M chrome/browser/ui/android/toolbar/java/src/org/chromium/chrome/browser/toolbar/top/tab_strip/HeightTransitionHandler.java
- M chrome/browser/ui/android/toolbar/java/src/org/chromium/chrome/browser/toolbar/top/tab_strip/TabStripTransitionCoordinatorUnitTest.java
Code-Review: +1 by Sirisha Kavuluru, +1 by Aishwarya Rajesh
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |