Skip to content

Commit

Permalink
[CT] Only send scroll signals once there has been a down scroll
Browse files Browse the repository at this point in the history
Bug: 1434438
Change-Id: Iac1c50ca2117d079bc71294e0bd7290dc0278c86
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4601835
Reviewed-by: Kevin Grosu <kgrosu@google.com>
Code-Coverage: Findit <findit-for-me@appspot.gserviceaccount.com>
Commit-Queue: Sinan Sahin <sinansahin@google.com>
Cr-Commit-Position: refs/heads/main@{#1155254}
  • Loading branch information
fsinan authored and Chromium LUCI CQ committed Jun 9, 2023
1 parent bd71711 commit 4aa7270
Show file tree
Hide file tree
Showing 2 changed files with 67 additions and 2 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -217,7 +217,8 @@ private void maybeStartSendingRealTimeEngagementSignals(Tab tab) {
@Override
public void onScrollStarted(
int scrollOffsetY, int scrollExtentY, boolean isDirectionUp) {
mScrollState.onScrollStarted(isDirectionUp);
// Only send the event if there has been a down scroll.
if (!mScrollState.onScrollStarted(isDirectionUp)) return;
// If we shouldn't send the real values, always send false.
notifyVerticalScrollEvent(mShouldSendRealValues && isDirectionUp);
}
Expand Down Expand Up @@ -374,6 +375,7 @@ static class ScrollState implements UserData {
int mScrollOffsetUpdateFrequency = NONE;
int mAfterScrollEndThresholdMs = DEFAULT_AFTER_SCROLL_END_THRESHOLD_MS;
Long mTimeLastOnScrollEnded;
boolean mHadFirstDownScroll;

/**
* @param frequency The {@link RootScrollOffsetUpdateFrequency.EnumType}, can be |NONE| or
Expand All @@ -386,13 +388,20 @@ void setParams(@EnumType int frequency, int afterScrollEndThreshold) {
mAfterScrollEndThresholdMs = afterScrollEndThreshold;
}

void onScrollStarted(boolean isDirectionUp) {
/**
* @param isDirectionUp Whether the scroll direction is up.
* @return Whether there has been a down scroll.
*/
boolean onScrollStarted(boolean isDirectionUp) {
// We shouldn't get an |onScrollStarted()| call while a scroll is still in progress,
// but it can happen. Call |onScrollEnded()| to make sure we're in a valid state.
if (mIsScrollActive) onScrollEnded(false);
mIsScrollActive = true;
mIsDirectionUp = isDirectionUp;
mTimeLastOnScrollEnded = null;
if (isDirectionUp && !mHadFirstDownScroll) return false;
mHadFirstDownScroll = true;
return true;
}

/**
Expand All @@ -405,6 +414,7 @@ boolean onScrollUpdate(int verticalScrollOffset, int maxVerticalScrollOffset) {
timeSinceLastOnScrollEndedMillis());
}
boolean validUpdateAfterScrollEnd = isValidUpdateAfterScrollEnd();
if (!mHadFirstDownScroll) return validUpdateAfterScrollEnd;
if (mIsScrollActive || validUpdateAfterScrollEnd) {
int scrollPercentage =
Math.round(((float) verticalScrollOffset / maxVerticalScrollOffset) * 100);
Expand All @@ -422,6 +432,9 @@ boolean onScrollUpdate(int verticalScrollOffset, int maxVerticalScrollOffset) {
*/
boolean onScrollDirectionChanged(boolean isDirectionUp) {
if (mIsScrollActive && isDirectionUp != mIsDirectionUp) {
// If the scroll direction changed, either the previous direction was or the new
// direction is a down scroll.
mHadFirstDownScroll = true;
mIsDirectionUp = isDirectionUp;
return true;
}
Expand Down Expand Up @@ -452,6 +465,7 @@ int onScrollEnded(boolean allowUpdateAfter) {
void resetMaxScrollPercentage() {
mMaxScrollPercentage = 0;
mMaxReportedScrollPercentage = 0;
mHadFirstDownScroll = false;
}

static @NonNull ScrollState from(Tab tab) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -762,6 +762,57 @@ public void pauseAndUnpauseSignalsOnPageWithTextFragment() {
verify(env.connection).notifyGreatestScrollPercentageIncreased(eq(env.session), eq(50));
}

@Test
@Features.EnableFeatures({ChromeFeatureList.CCT_REAL_TIME_ENGAGEMENT_SIGNALS_ALTERNATIVE_IMPL})
public void doesNotSendSignalsBeforeDownScroll() {
initializeTabForTest();
GestureStateListener listener = captureGestureStateListener(ON_SCROLL_END);

// Assume we started further down on the page and scroll up.
listener.onScrollStarted(50, SCROLL_EXTENT, true);
when(mRenderCoordinatesImpl.getScrollYPixInt()).thenReturn(30);
listener.onScrollOffsetOrExtentChanged(30, SCROLL_EXTENT);
listener.onScrollEnded(30, SCROLL_EXTENT);
// We shouldn't get any signals.
verify(env.connection, never()).notifyVerticalScrollEvent(eq(env.session), anyBoolean());
verify(env.connection, never())
.notifyGreatestScrollPercentageIncreased(eq(env.session), anyInt());
// Now scroll down from here.
listener.onScrollStarted(30, SCROLL_EXTENT, false);
when(mRenderCoordinatesImpl.getScrollYPixInt()).thenReturn(45);
listener.onScrollOffsetOrExtentChanged(45, SCROLL_EXTENT);
listener.onScrollEnded(45, SCROLL_EXTENT);
// We should get signals as if we've only scrolled down to this %.
verify(env.connection).notifyVerticalScrollEvent(eq(env.session), eq(false));
verify(env.connection).notifyGreatestScrollPercentageIncreased(eq(env.session), eq(45));
}

@Test
@Features.EnableFeatures({ChromeFeatureList.CCT_REAL_TIME_ENGAGEMENT_SIGNALS_ALTERNATIVE_IMPL})
public void doesNotSendSignalsBeforeDownScroll_AfterNavigation() {
initializeTabForTest();
GestureStateListener listener = captureGestureStateListener(ON_SCROLL_END);

// Scroll down.
listener.onScrollStarted(0, SCROLL_EXTENT, false);
when(mRenderCoordinatesImpl.getScrollYPixInt()).thenReturn(25);
listener.onScrollOffsetOrExtentChanged(25, SCROLL_EXTENT);
listener.onScrollEnded(25, SCROLL_EXTENT);
// We should get signals as usual.
verify(env.connection).notifyVerticalScrollEvent(eq(env.session), eq(false));
verify(env.connection).notifyGreatestScrollPercentageIncreased(eq(env.session), eq(25));
// Now, navigate to another page.
WebContentsObserver webContentsObserver = captureWebContentsObserver();
LoadCommittedDetails details = new LoadCommittedDetails(0, JUnitTestGURLs.getGURL(URL_1),
false, /*isSameDocument=*/false, /*isMainFrame=*/true, 200);
webContentsObserver.navigationEntryCommitted(details);
// Scroll up from some point in the page, e.g. back navigation or anchor fragment on page.
// We shouldn't get any (more) signals.
verify(env.connection, times(1)).notifyVerticalScrollEvent(eq(env.session), anyBoolean());
verify(env.connection, times(1))
.notifyGreatestScrollPercentageIncreased(eq(env.session), anyInt());
}

private void advanceTime(long millis) {
SystemClock.setCurrentTimeMillis(CURRENT_TIME_MS + millis);
}
Expand Down

0 comments on commit 4aa7270

Please sign in to comment.