Commit 644ce1b3 authored by Drew Willcoxon's avatar Drew Willcoxon
Browse files

Bug 1744367 - Part 1: Always make room for the heuristic result when maxRichResults > 0. r=harry 

The problem (as described in the bug) is that the user's `maxRichResults` is 3
and the tab-to-search provider returns two onboarding results, so when the muxer
subtracts the total result span (4) of those two results from the available span
(3), there's no room left for the heuristic. On top of that, there's the usual
async nature of fetching results where the muxer's `sort()` gets called multiple
times per query, so the heuristic can briefly appear before the TTS results
arrive and push it out.

This part-1 patch forces the available result span to be no smaller than the
largest heuristic result span when `maxRichResults` is positive. That way a
heuristic will always be included regardless of whatever suggested-index results
are present or whatever result spans the heuristic and suggested-index results
have.

This also fixes a potential other problem when the heuristic is hidden but has a
result span larger than 1. Currently we just increment the available span to
compensate, but it should be increased by the heuristic's result span.

For both of the two fixes mentioned above, this patch uses the largest heuristic
result span when there are multiple heuristics. The muxer can't know which
heuristic will end up winning in the end at the time it needs to make these
adjustments to the available span, so it could be the case that the largest
heuristic span is not the span of the final heuristic. But in that case the only
consequence is that the list of results will have fewer results than necessary.

Differential Revision: https://phabricator.services.mozilla.com/D134771
parent 0d647206

browser/components/urlbar/UrlbarMuxerUnifiedComplete.jsm

+42 −14
Original line number Diff line number Diff line
@@ -82,14 +82,10 @@ class MuxerUnifiedComplete extends UrlbarMuxer { 
      suggestions: new Set(),

      canAddTabToSearch: true,

      hasUnitConversionResult: false,

      maxHeuristicResultSpan: 0,

      // When you add state, update _copyState() as necessary.

    };



    // If the heuristic is hidden, increment the available span.

    if (UrlbarPrefs.get("experimental.hideHeuristic")) {

      state.availableResultSpan++;

    }



    // Do the first pass over all results to build some state.

    for (let result of context.results) {

      // Add each result to the appropriate `resultsByGroup` map.
@@ -109,6 +105,23 @@ class MuxerUnifiedComplete extends UrlbarMuxer { 
      this._updateStatePreAdd(result, state);

    }



    // Now that the first pass is done, adjust the available result span.

    if (state.maxHeuristicResultSpan) {

      if (UrlbarPrefs.get("experimental.hideHeuristic")) {

        // The heuristic is hidden. The muxer will include it but the view will

        // hide it. Increase the available span to compensate so that the total

        // visible span accurately reflects `context.maxResults`.

        state.availableResultSpan += state.maxHeuristicResultSpan;

      } else if (context.maxResults > 0) {

        // `context.maxResults` is positive. Make sure there's room for the

        // heuristic even if it means exceeding `context.maxResults`.

        state.availableResultSpan = Math.max(

          state.availableResultSpan,

          state.maxHeuristicResultSpan

        );

      }

    }



    // Determine the result groups to use for this sort.  In search mode with

    // an engine, show search suggestions first.

    let rootGroup = context.searchMode?.engineName
@@ -123,7 +136,13 @@ class MuxerUnifiedComplete extends UrlbarMuxer { 
      state

    );



    // Add global suggestedIndex results.

    // Add global suggestedIndex results unless the max result count is zero,

    // which isn't really supported but it's easy to honor here. We add them all

    // even if they exceed the max because we assume they're high-priority

    // results that should always be shown, and as long as the max is positive

    // it's not a problem to exceed it sometimes. In practice that will happen

    // only for small, non-default values of `maxRichResults`.

    if (context.maxResults > 0) {

      let suggestedIndexResults = state.resultsByGroup.get(

        UrlbarUtils.RESULT_GROUP.SUGGESTED_INDEX

      );
@@ -134,6 +153,7 @@ class MuxerUnifiedComplete extends UrlbarMuxer { 
          state

        );

      }

    }



    context.results = sortedResults;

  }
@@ -884,6 +904,14 @@ class MuxerUnifiedComplete extends UrlbarMuxer { 
   *   Global state that we use to make decisions during this sort.

   */

  _updateStatePreAdd(result, state) {

    // Keep track of the largest heuristic result span.

    if (result.heuristic && this._canAddResult(result, state)) {

      state.maxHeuristicResultSpan = Math.max(

        state.maxHeuristicResultSpan,

        UrlbarUtils.getSpanForResult(result)

      );

    }



    // Subtract from `availableResultSpan` the span of global suggestedIndex

    // results so there will be room for them at the end of the sort.

    if (

browser/components/urlbar/tests/unit/test_muxer.js

+125 −0
Original line number Diff line number Diff line
@@ -590,3 +590,128 @@ async function doBadHeuristicGroupsTest(resultGroups, expectedResults) { 


  Services.prefs.clearUserPref("browser.urlbar.resultGroups");

}



// When `maxRichResults` is positive and taken up by suggested-index result(s),

// both the heuristic and suggested-index results should be included because we

// (a) make room for the heuristic and (b) assume all suggested-index results

// should be included even if it means exceeding `maxRichResults`. The specified

// `maxRichResults` span will be exceeded in this case.

add_task(async function roomForHeuristic_suggestedIndex() {

  let results = [

    Object.assign(

      new UrlbarResult(

        UrlbarUtils.RESULT_TYPE.URL,

        UrlbarUtils.RESULT_SOURCE.HISTORY,

        { url: "http://example.com/heuristic" }

      ),

      { heuristic: true }

    ),

    Object.assign(

      new UrlbarResult(

        UrlbarUtils.RESULT_TYPE.URL,

        UrlbarUtils.RESULT_SOURCE.HISTORY,

        { url: "http://example.com/suggestedIndex" }

      ),

      { suggestedIndex: 1 }

    ),

  ];



  UrlbarPrefs.set("maxRichResults", 1);



  let provider = registerBasicTestProvider(results);

  let context = createContext(undefined, { providers: [provider.name] });

  await check_results({

    context,

    matches: results,

  });



  UrlbarPrefs.clear("maxRichResults");

});



// When `maxRichResults` is positive but less than the heuristic's result span,

// the heuristic should be included because we make room for it even if it means

// exceeding `maxRichResults`. The specified `maxRichResults` span will be

// exceeded in this case.

add_task(async function roomForHeuristic_largeResultSpan() {

  let results = [

    Object.assign(

      new UrlbarResult(

        UrlbarUtils.RESULT_TYPE.URL,

        UrlbarUtils.RESULT_SOURCE.HISTORY,

        { url: "http://example.com/heuristic" }

      ),

      { heuristic: true, resultSpan: 2 }

    ),

  ];



  UrlbarPrefs.set("maxRichResults", 1);



  let provider = registerBasicTestProvider(results);

  let context = createContext(undefined, { providers: [provider.name] });

  await check_results({

    context,

    matches: results,

  });



  UrlbarPrefs.clear("maxRichResults");

});



// When `maxRichResults` is zero and there are no suggested-index results, the

// heuristic should not be included.

add_task(async function roomForHeuristic_maxRichResultsZero() {

  let results = [

    Object.assign(

      new UrlbarResult(

        UrlbarUtils.RESULT_TYPE.URL,

        UrlbarUtils.RESULT_SOURCE.HISTORY,

        { url: "http://example.com/heuristic" }

      ),

      { heuristic: true }

    ),

  ];



  UrlbarPrefs.set("maxRichResults", 0);



  let provider = registerBasicTestProvider(results);

  let context = createContext(undefined, { providers: [provider.name] });

  await check_results({

    context,

    matches: [],

  });



  UrlbarPrefs.clear("maxRichResults");

});



// When `maxRichResults` is zero and suggested-index results are present,

// neither the heuristic nor the suggested-index results should be included.

add_task(async function roomForHeuristic_maxRichResultsZero_suggestedIndex() {

  let results = [

    Object.assign(

      new UrlbarResult(

        UrlbarUtils.RESULT_TYPE.URL,

        UrlbarUtils.RESULT_SOURCE.HISTORY,

        { url: "http://example.com/heuristic" }

      ),

      { heuristic: true }

    ),

    Object.assign(

      new UrlbarResult(

        UrlbarUtils.RESULT_TYPE.URL,

        UrlbarUtils.RESULT_SOURCE.HISTORY,

        { url: "http://example.com/suggestedIndex" }

      ),

      { suggestedIndex: 1 }

    ),

  ];



  UrlbarPrefs.set("maxRichResults", 0);



  let provider = registerBasicTestProvider(results);

  let context = createContext(undefined, { providers: [provider.name] });

  await check_results({

    context,

    matches: [],

  });



  UrlbarPrefs.clear("maxRichResults");

});