1395010 - Find API - Introduce a resultId

Status: NEW

(WebExtensions :: General, enhancement, P5)

Description

[Tim Nguyen :ntim]

Thinking more about the Find API, I feel like each item in rectData and rangeData should have a resultId.
 
Right now, they're both arrays, and they are linked by index, which isn't quite explicit.

Another reason I think this is a good idea would be for find events (bug 1395009).

Comment 1

[Tim Nguyen :ntim]

To be clear, there is a resultId at the moment, which is the index of each result in the array.

The same index is used in "highlightResult" with rangeIndex as parameter name.

My suggestion is simply to add the resultId to each item in rectData/rangeData, and rename the rangeIndex parameter to "resultId".

Comment 2

[Caitlin Neiman [:caitmuenster]]

Hi Tim, this has been added to the agenda for the October 17 WebExtensions APIs triage meeting. Would you be able to join us? 

Here's a few things about the triage to keep in mind: 

* We normally spend 5 minutes per bug
* The more information in the bug, the better
* The goal of the triage is to give a general thumbs up or thumbs down on a proposal; we won't be going deep into implementation details

Relevant Links: 

* Wiki for the meeting: https://wiki.mozilla.org/Add-ons/Contribute/Triage
* Meeting agenda: https://docs.google.com/document/d/1oUFGD57_NGbtV5y8k_yIS3GN8pFO3M_K1SWQhzlq6Ho/edit#heading=h.hhpni8ijl0wx
* Vision doc for WebExtensions: https://wiki.mozilla.org/WebExtensions/Vision

Comment 3

[Andy McKay]

We weren't sure what to do here and figured we should probably defer this to Kevin.

Comment 4

[u462496]

(In reply to Andy McKay [:andym] from comment #3)
> We weren't sure what to do here and figured we should probably defer this to
> Kevin.

I read the meeting notes and I agree that unless there is a compelling use case, I don't see a reason to change anything.  I think Will did a good job in the docs of making the relationship between rangeData and rectData clear, and I'm not convinced that there is inherently any confusion over this.

I'm not obstinately opposed to implementing this, and would have been open to the suggestion when the API was originally written, but at this moment in time I just haven't heard an argument that compels me to think that we should land more bugs and change the structure.

ni?ing Tim to provide a use case which would better help us understand the need to change this.

Comment 5

[Tim Nguyen :ntim]

I think a result id (could be anything, even just the array index) adds a certain coherence to the API. Right now, the relationship between rangeData and rectData might be clear, but when adding more methods that relate to a single result, it gets harder to understand.

Let's say we add support for find events (bug 1395009), it would be nice to be able to link the result in the event listener to the results found with browser.find.find(). It would be very hard to do without a resultId. 

The index of the result found with browser.find.find() is not enough here, because it doesn't work with duplicates, and you wouldn't be able to know the "resultId" without having ran browser.find.find() first.

Having an explicit resultId makes the relationship much easier to understand.

Also, designing the API like this is also more consistent with most APIs: tabs API with tabIds that work with all methods, bookmarks API with bookmarkIds, window API, ... and so on.

Comment 6

[u462496]

(In reply to Tim Nguyen :ntim from comment #5)

I'm still not hearing a concrete use case here, mainly just hypothetical stuff.

> I think a result id (could be anything, even just the array index) adds a
> certain coherence to the API. Right now, the relationship between rangeData
> and rectData might be clear, but when adding more methods that relate to a
> single result, it gets harder to understand.
> 
> Let's say we add support for find events (bug 1395009), it would be nice to
> be able to link the result in the event listener to the results found with
> browser.find.find(). It would be very hard to do without a resultId. 

As it stands now, there is no direct relationship between data returned from the find API and what occurs when a user uses the native Find feature.  These are kept separate by design, at least for now.  To establish congruence between the two, without one walking over the other would involve a good deal of complexity.  If one wants to mimic the behavior of say Findbar Tweak, they can listen for events via bug 1395009 which at minimum I hope will return the phrase used in the search along with params such as case sensitive etc, and repeat the action with browser.find API.  IIRC, this is essentially what Findbar Tweak does using its own instance of nsIFind, nsISelectionController, etc. in its legacy way.

> The index of the result found with browser.find.find() is not enough here,
> because it doesn't work with duplicates, and you wouldn't be able to know
> the "resultId" without having ran browser.find.find() first.

What do you mean by duplicates?  Can you give an example where indices for rangeData and rectData would not line up?

> Having an explicit resultId makes the relationship much easier to understand.
> 
> Also, designing the API like this is also more consistent with most APIs:
> tabs API with tabIds that work with all methods, bookmarks API with
> bookmarkIds, window API, ... and so on.

As I said, would have been good to have dealt with this on the onset, but nothing so far is compelling me to write new patches and tests and go through more review cycles over this.  AFAICT, there is nothing to keep an addon from iterating though the returned arrays and assigning their own resultIds if they want them.

Incidentally, are you working on bringing Findbar Tweak to a WebExtension?

Comment 7

[u462496]

Tim, can you give us an outline of how you see the data structure of rectData and rangeData using an id?

Comment 8

[Tim Nguyen :ntim]

(In reply to Kevin Jones from comment #7)
> Tim, can you give us an outline of how you see the data structure of
> rectData and rangeData using an id?

This is a tricky question, I would say:

[{ rectData: ..., rangeData: ..., resultId: ... }, { rectData: ..., rangeData: ..., resultId: ... }, ...]

but of course we can't change the API at this point.


So something like this:

{ rectData: [], rangeData: [], id: [] }

Comment 9

[u462496]

(In reply to Tim Nguyen :ntim from comment #8)
> So something like this:
> 
> { rectData: [], rangeData: [], id: [] }

Wouldn't this just always result in an array whose members == the indices?