1699607 - Muxer needs to take result spans into account

Status: RESOLVED WONTFIX

(Firefox :: Address Bar, defect, P3)

Description

[Drew Willcoxon :adw]

(In reply to Drew Willcoxon :adw from bug 1699211 comment #3)

• The muxer assumes each result's span is 1 when it fills buckets. That means it can include too many results, and whichever results happen to be over the max count are truncated by the providers manager after the mux. It needs to call UrlbarUtils.getSpanForResult. This problem by itself doesn't cause the flicker because the correct number of results is still returned due to the providers manager truncation, and the results are still consistent, they just don't have the correct mix, so it might be better to fix this in another bug, not sure yet.
  • One consequence of this is that our suggestedIndex logic is broken. Result truncation happens in the providers manager after the muxer sort, but suggestedIndex is handled in the muxer. If a result has a suggestedIndex that's > the index of a result with a > 1 result span, then the providers manager may truncate it. We never noticed this because until quick suggest, we never had a result with a large suggestedIndex. A result with suggestedIndex == maxResults - 1, like QS, is the most likely to be truncated.

Comment 1

[Drew Willcoxon :adw]

Attachment: Bug 1699607 - Modify the urlbar muxer to take result spans into account.

This looks a little worse than it is because a lot of it is renaming "result
count" identifiers to "result span". The core changes are in _addResults,
where it now checks result spans instead of result counts.

Comment 2

[Drew Willcoxon :adw]

Marco, I'm thinking about wontfixing this and I'd like your opinion. In summary, there's no single right way for the muxer to take result spans into account. In addition, the problem for quick suggest is that there's no reliable way to set the suggested index to the last index, due to result spans in earlier results, and that problem is orthogonal to this bug. (I flagged you for review on my fix for that in D109571.) Details below.

My patch for this bug (D109465) makes the muxer treat the max result count as the max result span count, and it strictly adheres to it, so if the muxer tries to add a result to a bucket but the bucket's new result span would be > its max result span, it doesn't add the result, and it stops filling that bucket. For example, the heuristic bucket has a max count of 1. If the muxer tries to add a result with span = 2, it can't, and the bucket remains empty. A search tips test fails because of that -- my patch breaks the heuritic search tip. So it can't be correct for the muxer to be so strict.

OK, the obvious solution is to allow buckets to be overfilled (according to result span, not result count). That's kind of what we have right now, but there's one important difference: truncation. Right now, buckets are overfilled (with respect to result span), but we don't try to compensate for overfill within the muxer. Instead, buckets/results are just pushed farther down the results list, and then the providers manager truncates the list. What we could do instead is to modify the muxer so it knows when previous buckets were overfilled so it can decrease the max result count of later buckets to compensate.

For example, right now if the buckets are suggestion:9,general:1 and the first suggestion has resultSpan = 2, the general result gets truncated by the providers manager. If the muxer were smarter, it could truncate the last suggestion on its own so that the general result is still included.

I'm not sure that's clearly better though. In both cases the results are valid, the compositions are just different. What do you think?

Comment 3

[Marco Bonardo [:mak]]

(In reply to Drew Willcoxon :adw from comment #2)

My patch for this bug (D109465) makes the muxer treat the max result count as the max result span count, and it strictly adheres to it, so if the muxer tries to add a result to a bucket but the bucket's new result span would be > its max result span, it doesn't add the result, and it stops filling that bucket. For example, the heuristic bucket has a max count of 1. If the muxer tries to add a result with span = 2, it can't, and the bucket remains empty.

yeah, this doesn't sound good, a result at index N should not be limited by span. That test was indeed a good alarm.

OK, the obvious solution is to allow buckets to be overfilled (according to result span, not result count). That's kind of what we have right now, but there's one important difference: truncation.

Also the fact you may have 11 spans of occupation, but the last result takes 2 spans, then if you truncate at 9, the urlbar shrinks, at 11 it grows, then what do you do? It depends, if the last result had a suggestedIndex, probably you want to remove the result before it, and let the last result stay, otherwise you may want to remove it and insert a spare 1-span result.
It's tricky from all the points of view :(

For example, right now if the buckets are suggestion:9,general:1 and the first suggestion has resultSpan = 2, the general result gets truncated by the providers manager. If the muxer were smarter, it could truncate the last suggestion on its own so that the general result is still included.

I'm not sure that's clearly better though. In both cases the results are valid, the compositions are just different. What do you think?

My above example is partially an answer, it depends on what is the result we are truncating. Is it a critical result? For example if it is a QS result we probably don't want to truncate it away. What if, as in my example, it's a result we'd truncate in the middle? It's likely a case we can't hit now because all of the multi-span results are at the top, so I'm mostly forward thinking.
The muxer should likely recognize important results that MUST stay, and drop before them if those could be pushed out.

That doesn't necessarily mean the muxer must count spans instead of results, but it must consider them to do a good job at preserving important results. I think the approach where we put down all the results and as the last step overwrite specific indices with suggestedIndex items works.

For the other cases, we don't yet have an idea of whether one result is more important than another one. In your example of 9 suggestions and 1 general, I have no idea if that 1 general is more or less important of the 9th suggestion, so how would we pick what to drop? We need some hint telling us what is more critical, and the only one we have is suggestedIndex. Once we'll have other hints, we can do better picks, but for now it's probably noncritical.

Comment 4

[Drew Willcoxon :adw]

Thanks. My assumption is that results with suggested indexes are always important and should never be removed/truncated. That's true for all suggested-index results currently: tips, interventions, quick suggest. Of course it may change in the future. My patch in D109571 always makes room for suggested-index results (except if there are so many of them that they overfill the entire results list), and it sounds like we agree on that.

I'll close this for now. We can always come back to it later.