1332144 - Find API

Status: RESOLVED FIXED

(WebExtensions :: General, defect)

Description

[u462496]

Two of my addons (All Tabs Helper / Hugo Search All Tabs) rely heavily on Finder.jsm methods and gFindBar.

In particular:

_getSelectionController
_getEditableNode
nsISelectionController.getSelection()

Also:

gFindBar.browser.contentWindow
gFindBar._findSelection

Requesting an applicable API so I can migrate these addons to WE.

Comment 1

[Andy McKay]

Could you suggest what an API for those might be like? And what they would do?

Comment 2

[u462496]

Attachment: Findbar_2.js

Comment 3

[u462496]

    (In reply to Andy McKay [:andym] from comment #1)
    > Could you suggest what an API for those might be like? And what they would
    > do?

    Probably easiest to use an example.

    I've uploaded a script you can paste into scratchpad, and run the Browser context.

    Find the section at the bottom of the script where you can enter 2 parameters:

    gQueryTerm - the phrase to search for
    gRangeIndex - the occurrence of the phrase you wish to be highlighted

    1) Load the active tab with a regular HTML page (must contain document.body)
    2) Enter parameters and run the script.

    You will see, given a query phrase, you can find all occurrences of the phrase on a page, and if you provide the index of which occurrence, that occurrence will be highlighted.

    NOTE 1: e10s must be turned *OFF* for this demo to work.
    NOTE 2: This demo will not search iFrames
    NOTE 3: Page loaded in the current tab must contain document.body
      

You have the following choices:

Comment 4

[Andy McKay]

The script didn't work for me. But I gave your add-ons a quick try instead. It sounds like you asking for an interface to the find method in a browser. So I'm going to make a wild guess that completely wrong:

browser.find.search(tabId, term): return an iterator of hits on the page
browser.find.highlight(tabId, term): highlight all the hits on a page
browser.find.reset(tabId): clear any highlights on a page

But we could also include more support for the find part:

browser.find.getText(): get the text entered in the find bar
browser.find.getSettings()... and so on.

I think if we can get an API description like that it might be easier to get an approval on. Personally that seems like a reasonable API to add and you seem to know this area very well. Would you be interested in porting some of the code you've already got into a WebExtension API?

Comment 5

[u462496]

(In reply to Andy McKay [:andym] from comment #4)
> The script didn't work for me.

You had e10s turned off, right?

> But I gave your add-ons a quick try instead.
> It sounds like you asking for an interface to the find method in a browser.

Basically, yes, though there seems to be some subtle differences to the user in Find and the APIs that I used in the script (and my addons).  For one, the highlighting is a different color for some reason.

> So I'm going to make a wild guess that completely wrong:
> 
> browser.find.search(tabId, term): return an iterator of hits on the page
> browser.find.highlight(tabId, term): highlight all the hits on a page

In addition eg:

browser.find.highlight(tabId, term, index): highlight a particular hit on a page

> browser.find.reset(tabId): clear any highlights on a page
> 
> But we could also include more support for the find part:
> 
> browser.find.getText(): get the text entered in the find bar
> browser.find.getSettings()... and so on.
> 
> I think if we can get an API description like that it might be easier to get
> an approval on. Personally that seems like a reasonable API to add and you
> seem to know this area very well. Would you be interested in porting some of
> the code you've already got into a WebExtension API?

I'm happy to help any way I'm able.

Comment 6

[u462496]

Attachment: Findbar_2.js revision 1

Comment 7

[u462496]

(In reply to Andy McKay [:andym] from comment #4)
> The script didn't work for me.

Sorry, there were a couple of bugs in the script.  I've uploaded a revision.

Comment 8

[u462496]

> > browser.find.highlight(tabId, term): highlight all the hits on a page
> 
> In addition eg:
> 
> browser.find.highlight(tabId, term, index): highlight a particular hit on a
> page

highlight would actually be better if you would only have to search for the hits once and can reuse the iterator:

let currentIterator;
browser.find.search(tabId, term).then((iterator) => {
  currentIterator = iterator;
});
...
// user clicks on a search result in the addon's results window for a given tab
browser.find.highlight(currentIterator, 0) // highlight single hit @ index 0
...
// user clicks on another search result in the addon's results window for the same tab
browser.find.highlight(currentIterator, 4) // highlight single hit @ index 4
...
// user clicks "highlight all" for the same tab
browser.find.highlight(currentIterator) // highlight all hits

Comment 9

[Andy McKay]

I saw some emails that you'd started and experiment on implementing this? My next suggestion was to play around with that, see what APIs make the most sense and then lets review that. Sometimes multiple comments in Bugzilla can get unwieldy for that sort of thing.

If that is the case, do you have a link to the code you are playing around with maybe on github, bitbucket or your favourite code hosting service?

Comment 10

[u462496]

(In reply to Andy McKay [:andym] from comment #9)
> I saw some emails that you'd started and experiment on implementing this? My
> next suggestion was to play around with that, see what APIs make the most
> sense and then lets review that. Sometimes multiple comments in Bugzilla can
> get unwieldy for that sort of thing.
> 
> If that is the case, do you have a link to the code you are playing around
> with maybe on github, bitbucket or your favourite code hosting service?

https://github.com/Allasso/API_find_git
https://github.com/Allasso/WE_find_git

WIP.  Just getting started with CVS so it's a bit stark right now.  I'll upload some docs soon I hope, though one should be able to get an idea looking at the code.

Comment 12

[Shane Caraveo (:mixedpuppy)]

Assigning to self to review all the details here.

Comment 13

[u462496]

Latest branch: https://github.com/Allasso/API_find_git/tree/rev_170221_1400
Use https://github.com/Allasso/WE_find_git addon to test.

This is very much a work in progress right now, extremely rough coming from a single dev context.  You may find some superfluous code, and dev specific debugging code (mainly dumps).

You will not get this running on your machine without some tweaking (hope to fix this soon):

  api.js - frameScriptFilename - set to the absolute path that is relevant to your machine.

All I can think of right now, hopefully there isn't more.

I will do some cleaning up and normalization as soon as I am able.

Not sure if I've done the Github thing right, but be sure to check the branch drop-down to make sure you're on the latest branch.  Please bear with me as I muddle through CVS.

Comment 14

[u462496]

Major updates, looking more like an API now than a dev minefield:

https://github.com/Allasso/API_find_git/tree/rev_170221_1400
Use https://github.com/Allasso/WE_find_git addon to test.

Shane, can you take a look at this and see how it's going?

Comment 15

[u462496]

(In reply to Kevin Jones from comment #14)
> Major updates, looking more like an API now than a dev minefield:
> 
> https://github.com/Allasso/API_find_git/tree/rev_170221_1400
> Use https://github.com/Allasso/WE_find_git addon to test.
> 
> Shane, can you take a look at this and see how it's going?

xpis included

Comment 16

[Tim Nguyen :ntim]

Kevin, thanks for the work on the API!

Most add-ons would like to hook within the find bar and new functionality to it. So may I suggest a way to set a callback on navigation:

browser.find.setNavigateCallback(function(index, result) {
  
});

This seems high-level enough to be integrated.

Comment 17

[u462496]

(In reply to Tim Nguyen :ntim from comment #16)
> Kevin, thanks for the work on the API!
> 
> Most add-ons would like to hook within the find bar and new functionality to
> it. So may I suggest a way to set a callback on navigation:
> 
> browser.find.setNavigateCallback(function(index, result) {
>   
> });
> 
> This seems high-level enough to be integrated.

Hello Tim, please forgive my ignorance, but I don't really know what you are saying here.  One of the considerations for this API is to hook into FindBar.jsm (maybe through gFindBar), but I don't understand what you are telling me with the setNavigateCallback function, and what you mean when you say "navigation"?

Comment 18

[Shane Caraveo (:mixedpuppy)]

Hi Kevin,
So first I'm just getting familiar with the code, these are some general initial comments.  

@Mike de Boer, can you take a short peek at the find code in browserScript.js and see if you have any comments?  See comment 14 for the github link.

schema.json:

- tabId rather than tabID
- use async: true rather than callbacks.  Other than chrome compat, we're solely using promises.  You don't need to define the callback function, but you do need the type returned as a param on the callback (which you have in types).
- tabId could be optional and just use the current tab if not provided.
- some indentation issues

api.js

- script loading might be a bit easier if you do something like this:

    let mm = window.getGroupMessageManager("browsers");
    mm.loadFrameScript("resource://extension-find-api/chrome/content/browserScript.js", true);

  However, it's fine what you're doing for now, we would change this later.

- loading services/modules (though you're not actually using services):

XPCOMUtils.defineLazyModuleGetter(this, "Services",
                                  "resource://gre/modules/Services.jsm");
getAPI(context) { 
  let {tabManager} = context.extension; 
  return {...}
}


browserScript.js

- defineLazyModuleGetter for Finder
- avoid creating an instance of rangeFinder till you need it, 

XPCOMUtils.defineLazyGetter(
  this, 'rangeFinder',
  function() {
    return Cc['@mozilla.org/embedcomp/rangefind;1'].createInstance(Ci.nsIFind);
  });

- docShell should be a global in content scripts
- You should be able to just use content.document rather than: this.content && this.content.document;
- You probably dont need doc to be an arg for findInDocument,  just use content.document in it.
- Same with passing window around (unless those are the window of a subframe?)

I'm guessing we'll want to integrate much of browserScript.js into toolkit/modules/RemoteFinder.jsm, having you thought about integration points yet?  Getting the message names/styles similar to what is used there would probably be good.

Comment 19

[Tim Nguyen :ntim]

(In reply to Kevin Jones from comment #17)
> (In reply to Tim Nguyen :ntim from comment #16)
> > Kevin, thanks for the work on the API!
> > 
> > Most add-ons would like to hook within the find bar and new functionality to
> > it. So may I suggest a way to set a callback on navigation:
> > 
> > browser.find.setNavigateCallback(function(index, result) {
> >   
> > });
> > 
> > This seems high-level enough to be integrated.
> 
> Hello Tim, please forgive my ignorance, but I don't really know what you are
> saying here.  One of the considerations for this API is to hook into
> FindBar.jsm (maybe through gFindBar), but I don't understand what you are
> telling me with the setNavigateCallback function, and what you mean when you
> say "navigation"?

When the user presses "up" and "down" in the findbar to navigate between results. There should be a way for add-ons to register an extra callback.

Comment 20

[Shane Caraveo (:mixedpuppy)]

(In reply to Tim Nguyen :ntim from comment #19)
> (In reply to Kevin Jones from comment #17)
> > (In reply to Tim Nguyen :ntim from comment #16)
> > > Kevin, thanks for the work on the API!
> > > 
> > > Most add-ons would like to hook within the find bar and new functionality to
> > > it. So may I suggest a way to set a callback on navigation:
> > > 
> > > browser.find.setNavigateCallback(function(index, result) {

> When the user presses "up" and "down" in the findbar to navigate between
> results. There should be a way for add-ons to register an extra callback.

I'm still confused.  If the callback is called for up/down movement, what new functionality can you add with it?  Maybe a use case here would help.

Comment 21

[u462496]

(In reply to Shane Caraveo (:mixedpuppy) from comment #18)
> Hi Kevin,
> So first I'm just getting familiar with the code, these are some general
> initial comments.  

Thanks Shane, good suggestions for a novice.

> schema.json:
> 
> - tabId rather than tabID
> - use async: true rather than callbacks.  Other than chrome compat, we're
> solely using promises.  You don't need to define the callback function, but
> you do need the type returned as a param on the callback (which you have in
> types).

Okay thanks, still trying to get familiar with how schema works...

> - tabId could be optional and just use the current tab if not provided.

I am doing that for search, however for highlightRanges it seemed necessary to keep things synced up, however maybe I'm just imagining improbable scenarios.

> api.js
> 
> - script loading might be a bit easier if you do something like this:
> 
>     let mm = window.getGroupMessageManager("browsers");
>    
> mm.loadFrameScript("resource://extension-find-api/chrome/content/
> browserScript.js", true);

Well, I was trying to avoid loading scripts unless they are needed.  I am not assuming that all browsers would necessarily be searched.

> - docShell should be a global in content scripts

Great!

> I'm guessing we'll want to integrate much of browserScript.js into
> toolkit/modules/RemoteFinder.jsm, having you thought about integration
> points yet?  Getting the message names/styles similar to what is used there
> would probably be good.

Hadn't even considered it, or that it was an option.  I'll look into how to do that.

Thanks Much!

Comment 22

[Shane Caraveo (:mixedpuppy)]

(In reply to Kevin Jones from comment #21)

> > mm.loadFrameScript("resource://extension-find-api/chrome/content/
> > browserScript.js", true);
> 
> Well, I was trying to avoid loading scripts unless they are needed.  I am
> not assuming that all browsers would necessarily be searched.

Yeah, later it would end up somewhere that would probably always be loaded.  I wouldn't worry about it right now.

> > I'm guessing we'll want to integrate much of browserScript.js into
> > toolkit/modules/RemoteFinder.jsm, having you thought about integration

> Hadn't even considered it, or that it was an option.  I'll look into how to
> do that.

You can't do that in an experiment, it's more just thinking about where the code would end up as a patch into Firefox.

Comment 23

[u462496]

(In reply to Shane Caraveo (:mixedpuppy) from comment #22) 
> You can't do that in an experiment, it's more just thinking about where the
> code would end up as a patch into Firefox.

I understood that, I was just considering that if it is integrated into remoteFinder.jsm, that the face of this code could change quite a bit.

Comment 24

[u462496]

BTW I updated the repo with most of your suggestions.  Still haven't changed tabId to being optional for highlightRanges, don't know if you saw my comment on that.  But if you still think it should be optional, I'll change it.

Comment 25

[Tim Nguyen :ntim]

(In reply to Shane Caraveo (:mixedpuppy) from comment #20)
> (In reply to Tim Nguyen :ntim from comment #19)
> > (In reply to Kevin Jones from comment #17)
> > > (In reply to Tim Nguyen :ntim from comment #16)
> > > > Kevin, thanks for the work on the API!
> > > > 
> > > > Most add-ons would like to hook within the find bar and new functionality to
> > > > it. So may I suggest a way to set a callback on navigation:
> > > > 
> > > > browser.find.setNavigateCallback(function(index, result) {
> 
> > When the user presses "up" and "down" in the findbar to navigate between
> > results. There should be a way for add-ons to register an extra callback.
> 
> I'm still confused.  If the callback is called for up/down movement, what
> new functionality can you add with it?  Maybe a use case here would help.

Findbar tweak [0] needs this to be able to integrate existing its results sidebar with the built-in findbar.

Eg. clicking up/down in the real findbar would be navigate up/down in the results sidebar.

The use-case here is really to extend the existing findbar functionality, rather than duplicating the findbar functionality inside a new UI.

[0]: https://addons.cdn.mozilla.net/user-media/previews/full/165/165756.png?modified=1485521459

Comment 26

[u462496]

(In reply to Tim Nguyen :ntim from comment #25)
> Findbar tweak [0] needs this to be able to integrate existing its results
> sidebar with the built-in findbar.
> 
> Eg. clicking up/down in the real findbar would be navigate up/down in the
> results sidebar.
> 
> The use-case here is really to extend the existing findbar functionality,
> rather than duplicating the findbar functionality inside a new UI.
> 
> [0]:
> https://addons.cdn.mozilla.net/user-media/previews/full/165/165756.
> png?modified=1485521459

It seems like if this is the extent of the requirement, we would only need messages sent for certain FindBar tasks.  But maybe I am not interpreting this use case correctly.

Is the addon implementing the highlighting in the sidebar based on the results of the FindBar tasks, or are you asking that the API highlight the sidebar results as well?

Comment 27

[Shane Caraveo (:mixedpuppy)]

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

> Findbar tweak [0] needs this to be able to integrate existing its results
> sidebar with the built-in findbar.

Ah...you want to provide results to searches in the findbar in your UI.  I might go a step further and also allow an extension to augment the findbar results.

I might suggest something like:

// return results to be added to the findbar
browser.find.onFindSearch.addListener((details) => {
  let results = []; // might be a list of objects with data for find
  // details = {
  //   searchText: "",
  // }
  // do something
  return results;
});

// handle extension ui changes based on events in findbar
browser.find.onFindEvent.addListener((details) => {
  let results = [];
  // not entirely sure what all needs to go here
  // details = {
  //   currentSelection: "",
  // }
  // do something
  return results;
});

Comment 28

[u462496]

(In reply to Kevin Jones from comment #26)
> (In reply to Tim Nguyen :ntim from comment #25)
> > Findbar tweak [0] needs this to be able to integrate existing its results
> > sidebar with the built-in findbar.
> > 
> > Eg. clicking up/down in the real findbar would be navigate up/down in the
> > results sidebar.
> > 
> > The use-case here is really to extend the existing findbar functionality,
> > rather than duplicating the findbar functionality inside a new UI.
> > 
> > [0]:
> > https://addons.cdn.mozilla.net/user-media/previews/full/165/165756.
> > png?modified=1485521459
> 
> It seems like if this is the extent of the requirement, we would only need
> messages sent for certain FindBar tasks.  But maybe I am not interpreting
> this use case correctly.
> 
> Is the addon implementing the highlighting in the sidebar based on the
> results of the FindBar tasks, or are you asking that the API highlight the
> sidebar results as well?

Maybe slightly off-topic question here, but it appears from the screenshot that FBT is digesting the results surrounded by a degree of context.  Is FBT getting the surrounding ranges by walking the DOM?

Comment 29

[Tim Nguyen :ntim]

(In reply to Shane Caraveo (:mixedpuppy) from comment #27)
> (In reply to Tim Nguyen :ntim from comment #25)
> 
> > Findbar tweak [0] needs this to be able to integrate existing its results
> > sidebar with the built-in findbar.
> 
> Ah...you want to provide results to searches in the findbar in your UI.  I
> might go a step further and also allow an extension to augment the findbar
> results.
>
> I might suggest something like:
> 
> // return results to be added to the findbar
> browser.find.onFindSearch.addListener((details) => {
>   let results = []; // might be a list of objects with data for find
>   // details = {
>   //   searchText: "",
>   // }
>   // do something
>   return results;
> });
> 
> // handle extension ui changes based on events in findbar
> browser.find.onFindEvent.addListener((details) => {
>   let results = [];
>   // not entirely sure what all needs to go here
>   // details = {
>   //   currentSelection: "",
>   // }
>   // do something
>   return results;
> });
The proposal looks great! I like the fact that we can add results to it.

Comment 30

[Shane Caraveo (:mixedpuppy)]

(In reply to Kevin Jones from comment #24)
> BTW I updated the repo with most of your suggestions.  Still haven't changed
> tabId to being optional for highlightRanges, don't know if you saw my
> comment on that.  But if you still think it should be optional, I'll change
> it.

It seems like it could be optional, but I am not sure what scenario you're considering where it shouldn't be.

Comment 31

[u462496]

(In reply to Shane Caraveo (:mixedpuppy) from comment #18)
> getAPI(context) { 
>   let {tabManager} = context.extension; 
>   return {...}
> }

Shane, I am not clear on what you are suggesting here.  Where does `context` come from/how is it accessed?  I have been poring over code and can't seem to get a handle on it.  As of FF 54 the way I've been accessing tabManager no longer works.

Comment 32

[u462496]

(In reply to Kevin Jones from comment #31)
> (In reply to Shane Caraveo (:mixedpuppy) from comment #18)
> > getAPI(context) { 
> >   let {tabManager} = context.extension; 
> >   return {...}
> > }
> 
> Shane, I am not clear on what you are suggesting here.  Where does `context`
> come from/how is it accessed?  I have been poring over code and can't seem
> to get a handle on it.  As of FF 54 the way I've been accessing tabManager
> no longer works.

Never mind, I got it worked out.

I also switched to using tabTracker instead of tabManager, as context.extension.tabManager does not provide the needed APIs.  I am importing ExtensionParent.jsm: ExtensionParent.apiManager.global.tabTracker.  I didn't see a way of accessing tabTracker via `context`.

Please see if this is acceptable to you:

https://github.com/Allasso/API_find_git/tree/rev_170221_1400

I will be working on integrating into RemoteFinder.jsm.

Comment 33

[Shane Caraveo (:mixedpuppy)]

Sorry for my late response, several times today I was in process of responding and got distracted.  Yes, tabTracker is what you want, and you got it the right way.  For future reference, context is BaseContext (some subclass of that depending on where its used).

Comment 34

[u462496]

Attachment: finder_remoteFinder_V1.diff

Integration of browser.find API methods into Finder.jsm and RemoteFinder.jsm.

I will be updating Experiments API with corresponding API shortly.

Comment 35

[u462496]

Comment on attachment 8843646 [details] [diff] [review]
finder_remoteFinder_V1.diff

Updated the API and test WE branches on GitHub to use with the patch.

Please use:

API:

https://github.com/Allasso/API_find_git/tree/API+Finder.jsm_RemoteFinder.jsm_patch

WE:

https://github.com/Allasso/WE_find_git/tree/WE-to-sync-with-API_find_git/tree/API+Finder.jsm_RemoteFinder.jsm_patch

Comment 36

[u462496]

(In reply to Shane Caraveo (:mixedpuppy) from comment #30)
> (In reply to Kevin Jones from comment #24)
> > BTW I updated the repo with most of your suggestions.  Still haven't changed
> > tabId to being optional for highlightRanges, don't know if you saw my
> > comment on that.  But if you still think it should be optional, I'll change
> > it.
> 
> It seems like it could be optional, but I am not sure what scenario you're
> considering where it shouldn't be.

I made this optional.

Comment 37

[u462496]

Tim, I am looking at the screenshot posted in comment 26.  I am interpreting your request that when the user makes a search using the native Findbar, that the results appear with surrounding context in the sidebar, is that correct?  Can you upload some example code showing how the XUL implementation of FBT is extending Firefox code?

Also, it appears from the that FBT is digesting the results surrounded by a degree of context.  Is FBT getting the surrounding ranges by walking the DOM?

Comment 38

[Tim Nguyen :ntim]

This is what the author uses: 

https://github.com/Quicksaver/FindBar-Tweak/blob/master/resource/modules/content/findInTabs.jsm#L514-L719

I'm not too familiar with the code, but it seems to maintain a map where the key is the rangeId, and the value the range itself.

Comment 39

[u462496]

Comment on attachment 8843646 [details] [diff] [review]
finder_remoteFinder_V1.diff

I am finding that my original proposal does not handle PDFs properly.  I also think it will be better to use nsITypeAheadFind methods as opposed to the nsIFind methods I proposed originally.

I am working on a patch for this now.  Marking 8843646 as obsolete.

Comment 40

[u462496]

Attachment: 1332144_full_api_V1.diff

Moved all of the code from an experiment into Firefox code.  Also the implementation has matured quite a bit.

WebExtension xpi for testing can be found here:

https://github.com/Allasso/WE_find_027_experiment_free

Comment 41

[Tim Nguyen :ntim]

Comment on attachment 8858975 [details] [diff] [review]
1332144_full_api_V1.diff

Review of attachment 8858975 [details] [diff] [review]:
-----------------------------------------------------------------

Mike should probably look at this too.

Comment 42

[Mike de Boer [:mikedeboer]]

Comment on attachment 8858975 [details] [diff] [review]
1332144_full_api_V1.diff

Review of attachment 8858975 [details] [diff] [review]:
-----------------------------------------------------------------

I'm generally in favor of having this functionality available as a WE API and I appreciate the move to using the pre-existing JSMs.

However, this proposed implementation re-implements already existing functionality (incompletely) and will suffer from performance issues. Don't worry, I'll explain everything in more detail below!
When you switch on the prefs `findbar.modalHighlighting` and `findbar.highlightAll`, you'll get a picture of the latest updates made to the finder as of late. The update wasn't just visually, but mostly under the hood:
 * The Highlight All implementation was naive and kicked of another iterator (RangeFinder) next to the other one counting occurrences of the active search term. This would result is excessive CPU usage and eventually blocking the content process.
 * New platform APIs were necessary to re-collect the rects and texts from a range, which may consist of multiple rects due to line breaks and/ or overflow. Range.getClientRectsAndTexts() was implemented for that very purpose. Another API is still in the making in bug 1302470 to make our findbar more future proof.
 * Search results and thus the highlighted ranges would not update when the content of the page changed. These type of dynamic re-runs of the iterator are now supported, but needed to be built.

All the above is implemented most prominently by the FinderIterator.jsm module. It is fully documented and tested and I tried to keep the size down a bit. Its most important feature is that it allows only one per-document find iterator to run at the same time to save resources. This means you should not run a search in multiple tabs in parallel, because that will not scale. On tab/ document at the time is my recommendation. The iterator implementation is stateful and re-entrant, which means you won't be hitting timing-related issues.
You can see how one (singleton) instance of FinderIterator.jsm is used by both Finder.jsm (for the word counter) and FinderHighlighter.jsm for the range highlighting.
This brings me to the second module: FinderHighlighter.jsm. All the editable node stuff and highlight-related methods have moved from Finder.jsm to there. I think your implementation would benefit a lot from using as much of this module as possible, by extending it with helpers to make it easier to use with multiple documents/ tabs at the same time. I also tried to provide as much documentation as possible for this module and it's also fully tested. By taking a look at the tests you could already get a feel for how you would be able to use the module standalone.

If you have any questions about the code, architecture, why or how things were implemented in a certain way, please let me know!

Comment 43

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8858975 [details] [diff] [review]
1332144_full_api_V1.diff

Matt, can you give feedback on the webext specific side of this?

Comment 44

[Mike de Boer [:mikedeboer]]

Ad: Kevin, we already know each other from plenty of other bugs (lazy tabs, \o/), so you already know that my turnaround time to getting to review and feedback requests is quite a bit shorter than was the case here (like 24hr-ish). The patch you attached here was substantial enough in size, functionality and complexity that I felt like I needed the time to really 'sit down' for this one. Perhaps for a next time, it'd be good to split up the patches into multiple parts - like commits - by theme/ topic? That way it becomes much more accessible to others who you'd like to provide feedback/ review them. Just a thought ;-)

Matt, the WebExtension surface area is the smaller part of this patch.

Comment 45

[u462496]

(In reply to Mike de Boer [:mikedeboer] from comment #44)
> Ad: Kevin, we already know each other from plenty of other bugs (lazy tabs,
> \o/), so you already know that my turnaround time to getting to review and
> feedback requests is quite a bit shorter than was the case here (like
> 24hr-ish). The patch you attached here was substantial enough in size,
> functionality and complexity that I felt like I needed the time to really
> 'sit down' for this one. Perhaps for a next time, it'd be good to split up
> the patches into multiple parts - like commits - by theme/ topic? That way
> it becomes much more accessible to others who you'd like to provide
> feedback/ review them. Just a thought ;-)
> 
> Matt, the WebExtension surface area is the smaller part of this patch.

Thank you Mike, that sounds good.  Re comment 42, lets see if I am getting this right.  It sounds like basically you are suggesting,

1) Run searches sequentially for each tab/document.
2) Move highlighting code to FinderHighlighter.jsm.

Is there anything else that I didn't pick up on?

I'll mention here also that I looked into PDF support, and that will be quite a project in itself (the PDF reader has its own search mechanism and tapping into that is not straightforward).  In this bug I am only focusing on XML, and I thought once this lands PDF support could be added in a different bug.

Comment 46

[u462496]

(In reply to Mike de Boer [:mikedeboer] from comment #42)
> Comment on attachment 8858975 [details] [diff] [review]
> 1332144_full_api_V1.diff
> 
> Review of attachment 8858975 [details] [diff] [review]:
> This means you
> should not run a search in multiple tabs in parallel, because that will not
> scale. On tab/ document at the time is my recommendation.

Are you referring to something specific in my code, or is this a general statement?  I am wondering because I am only running one tab at a time.

Comment 47

[Matthew Wein [:mattw]]

Comment on attachment 8858975 [details] [diff] [review]
1332144_full_api_V1.diff

Review of attachment 8858975 [details] [diff] [review]:
-----------------------------------------------------------------

In general I think the WebExtension side looks good. Thank you for documenting ext-find.js! Most of my comments are nits. Feeback+ with comments resolved.

::: browser/components/extensions/ext-browser.js
@@ +221,5 @@
>      paths: [
>        ["windows"],
>      ],
>    },
> +  find: {

nit: Please try to keep the module object sorted alphabetically.

::: browser/components/extensions/ext-find.js
@@ +5,5 @@
> +this.find = class extends ExtensionAPI {
> +  getAPI(context) {
> +    return {
> +      find: {
> +

nit: I don't think the extra line is needed here.

@@ +9,5 @@
> +
> +        /**
> +         * browser.find.search
> +         *
> +         * @param queryphrase string required - the string to search for.

nit: Please use jsdoc param format - http://usejsdoc.org/tags-param.html

@@ +17,5 @@
> +         *
> +         * Collects all ranges of queryphrase found within the document and its frames.
> +         * Stores collected ranges in array accessible by other browser.find methods.
> +         *
> +         * The returned value contains the tabId of the tabs searched, and an object

nit: Please use jsdoc @returns format - http://usejsdoc.org/tags-returns.html

@@ +31,5 @@
> +          let { queryphrase, tabId, includeRectData, includeRangeData } = query;
> +          let tab = tabId ? tabTracker.getTab(tabId) : tabTracker.activeTab;
> +          let browser = tab.linkedBrowser;
> +          let finder = browser.finder;
> +          tabId = tabTracker.getId(tab);

Why do you change the tabId here? I think this line should probably be removed.

@@ +51,5 @@
> +        },
> +
> +        /**
> +         * browser.find.highlightResults
> +         *

nit: Please follow http://usejsdoc.org/tags-param.html#parameters-with-properties for documenting parameters with properties.

@@ +65,5 @@
> +         *   2 - there were no search results at some of the indices supplied.
> +         *   3 - there were no search results at any of the indices supplied.
> +         *   4 - there were no search results to highlight.
> +         */
> +        highlightResults(params) {

nit: The code for search and highlightResults is very similar, so it might be less bug-prone to have the shared code moved to a helper.

@@ +88,5 @@
> +        },
> +
> +        /**
> +         * browser.find.removeHighlighting
> +         *

nit: Missing @param for the tabId.

::: browser/components/extensions/schemas/find.json
@@ +6,5 @@
> +  {
> +    "namespace": "manifest",
> +    "types": [
> +      {
> +        "$extend": "Permission",

It might be a good idea to have this extend "OptionalPermission".

@@ +18,5 @@
> +    ]
> +  },
> +  {
> +    "namespace": "find",
> +    "description": "Use the <code>chrome.find</code> API to interact with the browsers <code>find</code> system.",

s/chrome/browser/

@@ +24,5 @@
> +    "functions": [
> +      {
> +        "name": "search",
> +        "type": "function",
> +        "async": true,

This should be "async": "callback", and you'll need to add an extra property to the schema to describe the return value - see http://searchfox.org/mozilla-central/source/browser/components/extensions/schemas/bookmarks.json#132 for an example.

@@ +25,5 @@
> +      {
> +        "name": "search",
> +        "type": "function",
> +        "async": true,
> +        "description": "Search for text and store in array",

Could you expand on this? What is stored in the array and how is it structured/sorted?

@@ +32,5 @@
> +            "name": "query",
> +            "type": "object",
> +            "description": "Search query parameters",
> +            "properties": {
> +              "queryphrase": {

Please add descriptions for all properties in the schema. It looks like you already have these descriptions in ext-find.js, so you will probably be able to copy and paste most of those.

@@ +37,5 @@
> +                "type": "string"
> +              },
> +              "tabId": {
> +                "type": "integer",
> +                "optional": true,

The description here should mention what happens if the tab ID isn't provided.

@@ +55,5 @@
> +      },
> +      {
> +        "name": "highlightResults",
> +        "type": "function",
> +        "async": true,

Same as find.search, this should be "async": "callback" and include information about the return value.

@@ +72,5 @@
> +                }
> +              },
> +              "tabId": {
> +                "type": "integer",
> +                "minimum": 0

Should this be optional to be consistent with find.search?

@@ +94,5 @@
> +        "description": "Remove all highlighting from previous searches.",
> +        "parameters": [
> +          {
> +            "name": "tabId",
> +            "type": "integer",

The description here should mention what happens if the tab ID isn't provided.

Comment 48

[Mike de Boer [:mikedeboer]]

(In reply to Kevin Jones from comment #46)
> Are you referring to something specific in my code, or is this a general
> statement?  I am wondering because I am only running one tab at a time.

General statement.

Comment 49

[u462496]

(In reply to Matthew Wein [:mattw] from comment #47)
> Comment on attachment 8858975 [details] [diff] [review]
> 1332144_full_api_V1.diff
> 
> Review of attachment 8858975 [details] [diff] [review]:
> -----------------------------------------------------------------

Thank you for taking a look at my patch Matthew.  Just a couple of comments:

> 
> @@ +31,5 @@
> > +          let { queryphrase, tabId, includeRectData, includeRangeData } = query;
> > +          let tab = tabId ? tabTracker.getTab(tabId) : tabTracker.activeTab;
> > +          let browser = tab.linkedBrowser;
> > +          let finder = browser.finder;
> > +          tabId = tabTracker.getId(tab);
> 
> Why do you change the tabId here? I think this line should probably be
> removed.

tabId is an optional parameter so that line is needed, though I suppose I could instead do:

tabId = tabId || tabTracker.getId(tab);

> @@ +24,5 @@
> > +    "functions": [
> > +      {
> > +        "name": "search",
> > +        "type": "function",
> > +        "async": true,
> 
> This should be "async": "callback", and you'll need to add an extra property
> to the schema to describe the return value - see
> http://searchfox.org/mozilla-central/source/browser/components/extensions/
> schemas/bookmarks.json#132 for an example.

I have been told by two other reviewers on other APIs to explicitly not use "async": "callback", since Mozilla wants to use promises instead of callbacks, the exception being that if we are mirroring a Chrome API, which we are not in this case.

Comment 50

[Matthew Wein [:mattw]]

(In reply to Kevin Jones from comment #49)
> (In reply to Matthew Wein [:mattw] from comment #47)
> > Comment on attachment 8858975 [details] [diff] [review]
> > 1332144_full_api_V1.diff
> > 
> > Review of attachment 8858975 [details] [diff] [review]:
> > -----------------------------------------------------------------
> 
> Thank you for taking a look at my patch Matthew.  Just a couple of comments:
> 
> > 
> > @@ +31,5 @@
> > > +          let { queryphrase, tabId, includeRectData, includeRangeData } = query;
> > > +          let tab = tabId ? tabTracker.getTab(tabId) : tabTracker.activeTab;
> > > +          let browser = tab.linkedBrowser;
> > > +          let finder = browser.finder;
> > > +          tabId = tabTracker.getId(tab);
> > 
> > Why do you change the tabId here? I think this line should probably be
> > removed.
> 
> tabId is an optional parameter so that line is needed, though I suppose I
> could instead do:
> 
> tabId = tabId || tabTracker.getId(tab);

I think this would be better. I'm not very familiar with `tabTracker` yet - if `tab.id` works please use that instead.

> 
> > @@ +24,5 @@
> > > +    "functions": [
> > > +      {
> > > +        "name": "search",
> > > +        "type": "function",
> > > +        "async": true,
> > 
> > This should be "async": "callback", and you'll need to add an extra property
> > to the schema to describe the return value - see
> > http://searchfox.org/mozilla-central/source/browser/components/extensions/
> > schemas/bookmarks.json#132 for an example.
> 
> I have been told by two other reviewers on other APIs to explicitly not use
> "async": "callback", since Mozilla wants to use promises instead of
> callbacks, the exception being that if we are mirroring a Chrome API, which
> we are not in this case.

Okay thanks for the explanation. My main concern was about schema validation, but I don't think we validate callback arguments, though I think we should eventually. I'm on board with using "async": true here.

Comment 51

[u462496]

(In reply to Matthew Wein [:mattw] from comment #50)
> (In reply to Kevin Jones from comment #49)
> > (In reply to Matthew Wein [:mattw] from comment #47)
> > > Comment on attachment 8858975 [details] [diff] [review]
> > > 1332144_full_api_V1.diff
> > > 
> > > Review of attachment 8858975 [details] [diff] [review]:
> > > -----------------------------------------------------------------
> > > > +          let { queryphrase, tabId, includeRectData, includeRangeData } = query;
> > > > +          let tab = tabId ? tabTracker.getTab(tabId) : tabTracker.activeTab;
> > > > +          let browser = tab.linkedBrowser;
> > > > +          let finder = browser.finder;
> > > > +          tabId = tabTracker.getId(tab);
> > > 
> > 
> > tabId = tabId || tabTracker.getId(tab);
> 
> I think this would be better. I'm not very familiar with `tabTracker` yet -
> if `tab.id` works please use that instead.

Thank you Matthew.  Unfortunately the tab object returned by tabTracker does not have an id property.  One must use tabTracker.getId(tab).

Comment 52

[u462496]

Attachment: bug_1332144_Finder_V1.diff

Comment 53

[u462496]

Attachment: bug_1332144_RemoteFinder_V1.diff

Comment 54

[u462496]

Attachment: bug_1332144_WebExtensions_API_V1.diff

Update per your recommendations.  I opted however to not try to combine code for find.search and find.highlightResults.  I changed those methods quite a bit and it seemed that the convoluted-ness that would result in trying to combine those would not really effect the stated purpose.

Comment 55

[Tim Nguyen :ntim]

The patches don't seem to implement what's mentioned in comment 27. Are you planning to take care of that as well ?

Comment 56

[u462496]

(In reply to Tim Nguyen :ntim (not accepting requests until 1st June) from comment #55)
> The patches don't seem to implement what's mentioned in comment 27. Are you
> planning to take care of that as well ?

I thought I had commented on this, but I don't find it, maybe I am thinking of what I had stated in the triage meeting.  Anyway, my proposal was to get this stuff landed, then when the dust settles implement those changes as well.

Comment 57

[u462496]

(In reply to Kevin Jones from comment #56)
> (In reply to Tim Nguyen :ntim (not accepting requests until 1st June) from
> comment #55)
> > The patches don't seem to implement what's mentioned in comment 27. Are you
> > planning to take care of that as well ?
> 
> I thought I had commented on this, but I don't find it, maybe I am thinking
> of what I had stated in the triage meeting.  Anyway, my proposal was to get
> this stuff landed, then when the dust settles implement those changes as
> well.

I'll add these patches include some of the infrastructure to support comment 27, eg, Finder.jsm > `collectRectData` and `collectRangeData`.

Comment 58

[u462496]

A new demo xpi has been uploaded for the last 3 patches:
https://github.com/Allasso/WE_find_027_experiment_free > WE_find_028_optimize.xpi

Comment 59

[Matthew Wein [:mattw]]

Comment on attachment 8867746 [details] [diff] [review]
bug_1332144_WebExtensions_API_V1.diff

Review of attachment 8867746 [details] [diff] [review]:
-----------------------------------------------------------------

The implementation is looking good! I just have a few more nits.

Have you written a test for this yet? I think it would be good to have one before it lands. I'm not sure what experience you have with writing WebExtension tests - if you need any help, please ping me on IRC (:mattw in #webextensions) and I will help you with that as much as I can. 

r- until tests are added.

::: browser/components/extensions/ext-find.js
@@ +7,5 @@
> +    return {
> +      find: {
> +        /**
> +         * browser.find.search
> +         * Collects all ranges of queryphrase found within the document and its frames.

nit: Could you please define how a range is structured here?

@@ +24,5 @@
> +         *                  Whether to return rectangle data.
> +         * @param {boolean} query.includeRangeData optional
> +         *                  Whether to return range data.
> +         *
> +         * Returns a promise that will be resolved when search is completed.

nit: Please format like "@returns {Object} a promise that will be ..."

@@ +26,5 @@
> +         *                  Whether to return range data.
> +         *
> +         * Returns a promise that will be resolved when search is completed.
> +         *
> +         * @param {integer} tabId

nit: Please only use @param for function parameters.

@@ +57,5 @@
> +              finder.getSearchResults(queryphrase,
> +                                      caseSensitive,
> +                                      entireWord,
> +                                      includeRangeData,
> +                                      includeRectData);

nit: Please format like:

finder.getSearchResults(
  queryphrase, caseSensitive, entireWord, includeRangeData, includeRectData);

Same for below.

@@ +86,5 @@
> +         *                  Don't scroll to highlighted item.
> +         * @param {boolean} params.noRemoveHighlighting optional
> +         *                  Don't remove previous highlighting.
> +         *
> +         * Returns a value describing the resulting status of the highlighting:

nit: Please format this like "@returns {integer} the resulting status of the highlighting. This will be one of: ..."

@@ +91,5 @@
> +         *   1 - success.
> +         *   2 - index supplied was out of range.
> +         *   3 - there were no search results to highlight.
> +         *
> +         * @param {integer} status

nit: this line can be removed once the @returns changes are made.

Comment 60

[u462496]

Attachment: bug_1332144_Finder_P2_V1.diff

Comment 61

[u462496]

Attachment: bug_1332144_RemoteFinder_P2_V1.diff

Comment 62

[u462496]

Attachment: bug_1332144_unit_tests_P2_V1.diff

Comment 63

[u462496]

Attachment: bug_1332144_WebExtensions_API_P2_V1.diff

Comment 64

[u462496]

I have updated the demo WebExtension to work better with the current patches:

https://github.com/Allasso/WE_find_032

You'll find a xpi in there as well.

Note that I have added demos for various highlight implementations.  Please find HIGHLIGHTING_STYLE in background.js.

There is LOTS of documentation in the code.

Comment 65

[Mike de Boer [:mikedeboer]]

Sorry for the delay here! I will look at the patches coming Monday. ;-)

Comment 66

[u462496]

(In reply to Mike de Boer [:mikedeboer] from comment #65)
> Sorry for the delay here! I will look at the patches coming Monday. ;-)

np :-).  It'll give me a chance to update the patch with a bugfix (simple 2 additional lines).

Comment 67

[u462496]

Attachment: bug_1332144_Finder_P2_V2.diff

Bugfix

Comment 68

[Mike de Boer [:mikedeboer]]

Comment on attachment 8870159 [details] [diff] [review]
bug_1332144_RemoteFinder_P2_V1.diff

Review of attachment 8870159 [details] [diff] [review]:
-----------------------------------------------------------------

What is this patch used for? Can you merge it with the WebExtension patch that uses this? I'm missing all context here, I'm afraid.

Comment 69

[u462496]

(In reply to Mike de Boer [:mikedeboer] from comment #68)
> Comment on attachment 8870159 [details] [diff] [review]
> bug_1332144_RemoteFinder_P2_V1.diff
> 
> Review of attachment 8870159 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> What is this patch used for? Can you merge it with the WebExtension patch
> that uses this? I'm missing all context here, I'm afraid.

This is kind of the link between the WE side and the native side.  I'll ask :mattw to review it.

Comment 70

[u462496]

Comment on attachment 8870159 [details] [diff] [review]
bug_1332144_RemoteFinder_P2_V1.diff

Matt, would you be willing to review this?  Mike felt this was more on the WebExtensions side of things.

Comment 71

[Mike de Boer [:mikedeboer]]

Comment on attachment 8870158 [details] [diff] [review]
bug_1332144_Finder_P2_V1.diff

Review of attachment 8870158 [details] [diff] [review]:
-----------------------------------------------------------------

I was hoping you'd find a way to re-use Finder.jsm, FinderIterator.jsm and FinderHighlighter.jsm by adding a setter for the `this.iterator` property, so that you can instantiate the class without the need to try so hard to not mess with the search results of the findbar, which uses the same thing.
For your use case I do think it's practical to be able to have a _separate_ instance of the FinderIterator so you could possible add a `clone` method in there that returns a copy of itself. What do you think?
But yeah, all that stuff runs in the content process, so that's where most of the heavy lifting will be done. In your extension code you can implement your own messages to implement the API entirely on the WebExtension side, without modifications to the Finder.jsm and FinderHighlighter modules. Just create a new instance of Finder.jsm with a clone of the FinderIterator set on it, dedicated to your API.
You can add listeners for all the things you want to know and react to by using `finder.addListener(this);`, which may be a simple object that can have the following methods: 'onFindResult', 'onCurrentSelection', 'onMatchesCountResult' and 'onHighlightFinished'. If you need something more granular, invoke `FinderIterator.start()` on your clone directly.

Comment 72

[Mike de Boer [:mikedeboer]]

(In reply to Kevin Jones from comment #69)
> This is kind of the link between the WE side and the native side.  I'll ask
> :mattw to review it.

No, sorry, what I meant was: because I don't see this code next the WebExtension API code, I can't really reason well enough about it. That's the context I'm missing. So if you can merge the toolkit and WE patch together into one, I'd really appreciate it!

Comment 73

[u462496]

(In reply to Mike de Boer [:mikedeboer] from comment #72)
> (In reply to Kevin Jones from comment #69)
> > This is kind of the link between the WE side and the native side.  I'll ask
> > :mattw to review it.
> 
> No, sorry, what I meant was: because I don't see this code next the
> WebExtension API code, I can't really reason well enough about it. That's
> the context I'm missing. So if you can merge the toolkit and WE patch
> together into one, I'd really appreciate it!

Okay, I think I see what you mean now.

Comment 74

[u462496]

(In reply to Mike de Boer [:mikedeboer] from comment #71)
> Comment on attachment 8870158 [details] [diff] [review]
> bug_1332144_Finder_P2_V1.diff
> 
> Review of attachment 8870158 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I was hoping you'd find a way to re-use Finder.jsm, FinderIterator.jsm and
> FinderHighlighter.jsm by adding a setter for the `this.iterator` property,
> so that you can instantiate the class without the need to try so hard to not
> mess with the search results of the findbar, which uses the same thing.
> For your use case I do think it's practical to be able to have a _separate_
> instance of the FinderIterator so you could possible add a `clone` method in
> there that returns a copy of itself. What do you think?
> But yeah, all that stuff runs in the content process, so that's where most
> of the heavy lifting will be done. In your extension code you can implement
> your own messages to implement the API entirely on the WebExtension side,
> without modifications to the Finder.jsm and FinderHighlighter modules. Just
> create a new instance of Finder.jsm with a clone of the FinderIterator set
> on it, dedicated to your API.
> You can add listeners for all the things you want to know and react to by
> using `finder.addListener(this);`, which may be a simple object that can
> have the following methods: 'onFindResult', 'onCurrentSelection',
> 'onMatchesCountResult' and 'onHighlightFinished'. If you need something more
> granular, invoke `FinderIterator.start()` on your clone directly.

I'll have to chew on all this a bit to get what all your saying.

Comment 75

[u462496]

Attachment: bug_1332144_WebExtensions_API_P2_V2.diff

Hope this is what you meant...

Comment 76

[u462496]

Attachment: bug_1332144_API_P3_V1.diff

Oh, I guess you wanted __all__ the toolkit stuff in there?

I'm sorry, I got confused about what you were asking in comment 44.

Comment 77

[Matthew Wein [:mattw]]

Hi Kevin, I just want to give you an update that I have been working on a review of the patches you've uploaded and will plan to have my review submitted early on in the week.

Comment 78

[u462496]

(In reply to Mike de Boer [:mikedeboer] from comment #71)
> Comment on attachment 8870158 [details] [diff] [review]
> bug_1332144_Finder_P2_V1.diff
> 
> Review of attachment 8870158 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I was hoping you'd find a way to re-use Finder.jsm, FinderIterator.jsm and
> FinderHighlighter.jsm by adding a setter for the `this.iterator` property,
> so that you can instantiate the class without the need to try so hard to not
> mess with the search results of the findbar, which uses the same thing.
> For your use case I do think it's practical to be able to have a _separate_
> instance of the FinderIterator so you could possible add a `clone` method in
> there that returns a copy of itself. What do you think?
> But yeah, all that stuff runs in the content process, so that's where most
> of the heavy lifting will be done. In your extension code you can implement
> your own messages to implement the API entirely on the WebExtension side,
> without modifications to the Finder.jsm and FinderHighlighter modules. Just
> create a new instance of Finder.jsm with a clone of the FinderIterator set
> on it, dedicated to your API.
> You can add listeners for all the things you want to know and react to by
> using `finder.addListener(this);`, which may be a simple object that can
> have the following methods: 'onFindResult', 'onCurrentSelection',
> 'onMatchesCountResult' and 'onHighlightFinished'. If you need something more
> granular, invoke `FinderIterator.start()` on your clone directly.

So are you saying, I would be injecting an additional (hopefully small) frame script that would import Finder.jsm, FinderIterator.jsm and FinderHighlighter.jsm, where I would be creating and running a separate instance of all the classes needed to do what I'm doing here?

> I was hoping you'd find a way to re-use Finder.jsm, FinderIterator.jsm and
> FinderHighlighter.jsm by adding a setter for the `this.iterator` property,

I'm not clear on what you are saying here - what would adding a setter here look like?

> You can add listeners for all the things you want to know and react to by
> using `finder.addListener(this);`, which may be a simple object that can

Are you saying you want to use listeners in place of returning a promise for the methods I am currently using?  I am not seeing the benefit in that.

Comment 79

[u462496]

(In reply to Matthew Wein [:mattw] from comment #77)
> Hi Kevin, I just want to give you an update that I have been working on a
> review of the patches you've uploaded and will plan to have my review
> submitted early on in the week.

Thanks Matthew!

Comment 80

[Matthew Wein [:mattw]]

Comment on attachment 8872675 [details] [diff] [review]
bug_1332144_API_P3_V1.diff

Review of attachment 8872675 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks!

::: browser/components/extensions/ext-find.js
@@ +26,5 @@
> +         *                  Whether to return range data.
> +         *
> +         * @returns a promise that will be resolved when search is completed.
> +         */
> +        search(query) {

The 'queryphrase' paremeter is more important than the others, and it's essential for this method to work, so I'm thinking it might improve your design to have it moved out as the first parameter, and having the rest be an optional object - like `search(queryphrase, options)`

@@ +86,5 @@
> +              mm.addMessageListener("Finder:HighlightSearchResultsFinished", function highlightFindRanges(message) {
> +                mm.removeMessageListener("Finder:HighlightSearchResultsFinished", highlightFindRanges);
> +                resolve(message.params);
> +              });
> +              finder.highlightSearchResults(params.rangeIndex, noScroll, noRemoveHighlighting);

Could you just pass `rangeIndex` here? Same below?

@@ +106,5 @@
> +          let tab = tabId ? tabTracker.getTab(tabId) : tabTracker.activeTab;
> +          return new Promise(resolve => {
> +            tab.linkedBrowser.finder.removeHighlighting();
> +            resolve();
> +          });

Functions return empty promises by default, so I think this could code be simplified to:

removeHighlighting(tabId) {
  let tab = tabId ? tabTracker.getTab(tabId) : tabTracker.activeTab;
  tab.linkedBrowser.finder.removeHighlighting();
}

Comment 81

[Matthew Wein [:mattw]]

Comment on attachment 8870161 [details] [diff] [review]
bug_1332144_unit_tests_P2_V1.diff

Review of attachment 8870161 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for working on this!

::: browser/components/extensions/test/browser/browser_ext_find.js
@@ +17,5 @@
> +  '  var r3 = range.getBoundingClientRect();' +
> +  '  var rect = { top: (r1.top + r2.top + r3.top), left: (r1.left + r2.left + r3.left) };' +
> +  '  this.sendAsyncMessage("test:find:selectionTest", { text: selection.toString(), rect });' +
> +  '};\n' +
> +  'getSelectedText();\n';

Could you write frameScript as a function here instead of as a string? I'm thinking something like this would be more maintainable:

function frameScript() {
 ... // body of getSelectedText()
}

...

let frameScriptUrl = `data:,(${frameScript})();`

@@ +48,5 @@
> +    await awaitLoad(tab.id);
> +
> +    let data = await browser.find.search({ queryphrase: "banana", includeRangeData: true });
> +    let rangeData = data.searchResults.rangeData
> +

Could you add a test for the length of `rangeData` here?

@@ +51,5 @@
> +    let rangeData = data.searchResults.rangeData
> +
> +    browser.test.log("Test that the text found in the top window corresponds to the proper position.");
> +    browser.test.assertEq(rangeData[1].text, "bAnana", "The text at range position 1:");
> +

The `rangeData` array seems comfusing to me. Is there nothing stored in rangeData[0], rangeData[2], and rangeData[4]? If not, I'm not sure I like having a sparse array returned.

@@ +58,5 @@
> +
> +    browser.test.log("Test that the text found in the second-level nested frame corresponds to the proper position.");
> +    browser.test.assertEq(rangeData[5].text, "bananA", "The text at range position 5:");
> +
> +    data = await browser.find.search({ queryphrase: "baNana", caseSensitive: true, includeRangeData: true });

Could you also add tests which explicitly set caseSensitive and includeRangeData to false?

@@ +65,5 @@
> +    browser.test.log("Test that case sensitive match works properly.");
> +    browser.test.assertEq(rangeData.length, 1, "The number of matches found:");
> +    browser.test.assertEq(rangeData[0].text, "baNana", "The text found:");
> +
> +    data = await browser.find.search({ queryphrase: "banana", entireWord: true, includeRangeData: true });

Could you add a test which uses entireWord but doesn't pass the entire word?

@@ +72,5 @@
> +    browser.test.log("Test that entire word match works properly.");
> +    browser.test.assertEq(rangeData.length, 4, "The number of matches found:");
> +
> +    data = await browser.find.search({ queryphrase: "banana", includeRectData: true });
> +    let result = await browser.find.highlightResults({ rangeIndex: 5 });

It looks like you started working on testing highlightResults but didn't get around to finishing it - it would be good to test the 3 return values that `highlightResults` can return.

@@ +95,5 @@
> +  let promise = waitForMessage(browser, "test:find:selectionTest");
> +
> +  let frameScriptUrl = 'data:,' + frameScript;
> +  browser.messageManager.loadFrameScript(frameScriptUrl, false);
> +  let message = await promise;

I think simplifying this to `let message = await waitForMessage(browser, "test:find:selectionTest");` would be fine here.

@@ +98,5 @@
> +  browser.messageManager.loadFrameScript(frameScriptUrl, false);
> +  let message = await promise;
> +
> +  info("Test that text was highlighted properly.");
> +  is (message.data.text, "bananA", `The text that was highlighted: - Expected: bananA, Actual: ${message.data.text}`);

I think `is(...) will log actual and expected, so I think just passing a message like "correct text was highlighted" would be better.

::: browser/components/extensions/test/browser/file_find_frames.html
@@ +1,4 @@
> +<!DOCTYPE html>
> +<html>
> +<body>
> +

How is the find API supposed to work for text that spans multiple tags - e.g. <p>ba<div>na</div>na</pa>? Should this return a result if "banana" is searched? Could you please add a test case for that?

@@ +1,5 @@
> +<!DOCTYPE html>
> +<html>
> +<body>
> +
> +<p>Banana 0</p>

Should there be a test for the text "Banana"?

Comment 82

[Mike de Boer [:mikedeboer]]

(In reply to Kevin Jones from comment #78)
> (In reply to Mike de Boer [:mikedeboer] from comment #71)
> > Comment on attachment 8870158 [details] [diff] [review]
> > bug_1332144_Finder_P2_V1.diff
> > 
> > Review of attachment 8870158 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > I was hoping you'd find a way to re-use Finder.jsm, FinderIterator.jsm and
> > FinderHighlighter.jsm by adding a setter for the `this.iterator` property,
> > so that you can instantiate the class without the need to try so hard to not
> > mess with the search results of the findbar, which uses the same thing.
> > For your use case I do think it's practical to be able to have a _separate_
> > instance of the FinderIterator so you could possible add a `clone` method in
> > there that returns a copy of itself. What do you think?
> > But yeah, all that stuff runs in the content process, so that's where most
> > of the heavy lifting will be done. In your extension code you can implement
> > your own messages to implement the API entirely on the WebExtension side,
> > without modifications to the Finder.jsm and FinderHighlighter modules. Just
> > create a new instance of Finder.jsm with a clone of the FinderIterator set
> > on it, dedicated to your API.
> > You can add listeners for all the things you want to know and react to by
> > using `finder.addListener(this);`, which may be a simple object that can
> > have the following methods: 'onFindResult', 'onCurrentSelection',
> > 'onMatchesCountResult' and 'onHighlightFinished'. If you need something more
> > granular, invoke `FinderIterator.start()` on your clone directly.
> 
> So are you saying, I would be injecting an additional (hopefully small)
> frame script that would import Finder.jsm, FinderIterator.jsm and
> FinderHighlighter.jsm, where I would be creating and running a separate
> instance of all the classes needed to do what I'm doing here?

Yes! So insert that frame script upon first use of the API.

> I'm not clear on what you are saying here - what would adding a setter here
> look like?

`new Finder().iterator = FinderIterator.clone()` or something similar.

> Are you saying you want to use listeners in place of returning a promise for
> the methods I am currently using?  I am not seeing the benefit in that.

Well, you're implementing new methods that more-or-less do what the Finder modules are doing already, albeit slightly differently. I'm asking for not simply adding a lot of new code to the modules, but instead refactor them to suit your usecase as well.
If it makes more sense to use Promises, please feel free to refactor things using those! It's certainly possible that whilst creating these modules we haven't thought of your usecase for them; I'd like to try if we can make it work that way somehow.

Comment 83

[u462496]

Attachment: bug_1332144_API_P4_V1.diff

Updated API patch with suggestions from Matthew and Mike.

Tests to come in separate patch.

Comment 84

[u462496]

Attachment: bug_1332144_API_P4_V2.diff

small update.

Comment 85

[u462496]

Attachment: bug_1332144_API_P4_V3.diff

...

Comment 86

[u462496]

Attachment: WE_API_find_demo_basic.zip

Demo WebExtension - Basic

Will demonstrate all features available from the Find API.

Comment 87

[u462496]

Attachment: WE_API_find_demo_advanced.zip

Demo WebExtension - Advanced

Will demonstrate all functionality offered by the Find API.

In addition, gives examples of utilizing rangeData and rectData returned by search operation to display custom UI features.  eg, get surrounding context of search results, and various custom highlighting examples, including "FindbarTweak" style highlighting animation.

See READMEs for each extension for detailed instructions.

Comment 88

[u462496]

Comment on attachment 8881173 [details] [diff] [review]
bug_1332144_API_P4_V3.diff

Matthew, just a note that I changed the params a bit and documentation on the WE side.

Comment 89

[u462496]

Attachment: bug_1332144_unit_tests_P4_V1.diff

Update patch for unit tests.

(In reply to Matthew Wein [:mattw] from comment #81)
> Comment on attachment 8870161 [details] [diff] [review]
> bug_1332144_unit_tests_P2_V1.diff
> 
> Review of attachment 8870161 [details] [diff] [review]:
> -----------------------------------------------------------------
> The `rangeData` array seems comfusing to me. Is there nothing stored in
> rangeData[0], rangeData[2], and rangeData[4]? If not, I'm not sure I like
> having a sparse array returned.
> ...
> Should there be a test for the text "Banana"?

Sorry that I made that all confusing.  No, the returned array is not sparse.  The test now iterates through all results and report the finding, so that should make things less confusing.

> Could you add a test which uses entireWord but doesn't pass the entire word?

Actually there was:  When searching for case-insensitive "banana", "banaNaland" and "bananAland" were not matched.  I changed the message to try to make it clearer.

> I think `is(...) will log actual and expected,

Actually, apparently that is not case with `is(...)`.

All other comments addressed and tests added where requested.

Thanks.

Comment 90

[Matthew Wein [:mattw]]

Comment on attachment 8881173 [details] [diff] [review]
bug_1332144_API_P4_V3.diff

Review of attachment 8881173 [details] [diff] [review]:
-----------------------------------------------------------------

This is looking good, but I think there's still a few changes that I think need to be made before I'll be comfortable r+'ing. I still need to review find-content.js, but will do that once the next patch is uploaded.

::: browser/components/extensions/ext-find.js
@@ +3,5 @@
> +"use strict";
> +
> +this.find = class extends ExtensionAPI {
> +  getAPI(context) {
> +    let loadedFrameScripts = {};

Will the frame scripts still be loaded in tabs if the extension is uninstalled? If so, I think you might want to do something like this:

// Map[TabID -> FrameScript]
let loadedFrameScripts = new Map();
getAPI(context) { ... }
onShutdown() {
  ...
  let mm = browser.messageManager;
  for (let frameScript of loadedFrameScripts.values()) {
    mm.removeDelayedFrameScript(frameScript);
  }
}
...

@@ +39,5 @@
> +        /**
> +         * browser.find.highlightResults
> +         * Highlights range(s) found in previous browser.find.search.
> +         *
> +         * @param {object} params optional - may contain any of the following properties:

properties: -> properties,

@@ +47,5 @@
> +         *             Default highlights all ranges.
> +         *   {integer} tabId - Tab to highlight.  Defaults to the active tab.
> +         *   {boolean} noScroll - Don't scroll to highlighted item.
> +         *
> +         * @returns a promise that will be resolved when highlighting is completed that includes an

To make this more consistent with our other APIs, I think we should use the integers returned to either resolve or reject the promise with appropriate messages. For example, something like:

this.runFindOperation(params, "HighlightSearchResults").then(result => {
  switch (result) {
    case 1:
      resolve();
      break;
    case 2:
      reject("index supplied was out of range");
      break;
    case 3:
      reject("no search results to highlight");
      break;
  }
});

@@ +48,5 @@
> +         *   {integer} tabId - Tab to highlight.  Defaults to the active tab.
> +         *   {boolean} noScroll - Don't scroll to highlighted item.
> +         *
> +         * @returns a promise that will be resolved when highlighting is completed that includes an
> +         *   integer value indicating the resulting status of the highlighting.  This will be one of:

This function currently doesn't resolve with an integer - it resolves with an object that has a "status" property which is an integer, and it looks like it also resolves with a "tabId" property. Going off of my comment above, I'm thinking it shouldn't resolve with anything, and reject with appropriate error messages if it fails.

@@ +93,5 @@
> +
> +          return new Promise(resolve => {
> +            mm.addMessageListener(`ext-Finder:${message}Finished`, function messageListener(message) {
> +              mm.removeMessageListener(`ext-Finder:${message}Finished`, messageListener);
> +              message.data.tabId = tabId;

Why are you adding the tab ID here?

Comment 91

[u462496]

(In reply to Matthew Wein [:mattw] from comment #90)
 
> Will the frame scripts still be loaded in tabs if the extension is
> uninstalled? If so, I think you might want to do something like this:
> 
> // Map[TabID -> FrameScript]
> let loadedFrameScripts = new Map();
> getAPI(context) { ... }
> onShutdown() {
>   ...
>   let mm = browser.messageManager;
>   for (let frameScript of loadedFrameScripts.values()) {
>     mm.removeDelayedFrameScript(frameScript);
>   }
> }
> ...

Hmmm, actually I think passing `true` for the `aAllowDelayedLoad` param in `mm.loadFrameScript();` was an oversight.  There is no reason to use delayed load here.

> @@ +93,5 @@
> > +
> > +          return new Promise(resolve => {
> > +            mm.addMessageListener(`ext-Finder:${message}Finished`, function messageListener(message) {
> > +              mm.removeMessageListener(`ext-Finder:${message}Finished`, messageListener);
> > +              message.data.tabId = tabId;
> 
> Why are you adding the tab ID here?

This allows addons to keep track of which tabs the results are for.  An addon (such as Hugo or All Tabs Helper) may do a bulk search of all loaded tabs.

Comment 92

[u462496]

^^^^

Comment 93

[u462496]

(In reply to Kevin Jones from comment #91)
> > Why are you adding the tab ID here?
> 
> This allows addons to keep track of which tabs the results are for.  An
> addon (such as Hugo or All Tabs Helper) may do a bulk search of all loaded
> tabs.

In addition, it provides convenience when methods are called on the active tab (default) by omitting tabId param.  The addon does not then have to run an explicit call to tabs.query to get the tabId.

Comment 94

[u462496]

Attachment: bug_1332144_API_P4_V10.diff

Comment 95

[u462496]

Attachment: bug_1332144_API_P4_V11.diff

I decided to eliminate the return of tabId altogether.

Comment 96

[Tim Nguyen :ntim]

Comment on attachment 8883161 [details] [diff] [review]
bug_1332144_API_P4_V11.diff

Review of attachment 8883161 [details] [diff] [review]:
-----------------------------------------------------------------

I was curious about the demos, so I tried to apply this patch.

I noticed that you forgot to register ext-find.js and find.json inside the jar.mn files at browser/components/extensions/jar.mn and browser/components/extensions/schemas/jar.mn

Other than that, I'm going to admit I couldn't get the demos working (but perhaps that's because noVNC is very slow).

Comment 97

[Mike de Boer [:mikedeboer]]

Comment on attachment 8883161 [details] [diff] [review]
bug_1332144_API_P4_V11.diff

Review of attachment 8883161 [details] [diff] [review]:
-----------------------------------------------------------------

My compliments, Kevin! Look at how minimal and nicely contained your implementation has become!

I apologize for the delay in getting to this, but if you address my one (simple) comment, I'd be happy to see this ship.
I haven't tried this IRL, so perhaps it'd be good to see why Tim couldn't get things to work...

::: browser/components/extensions/ext-find.js
@@ +52,5 @@
> +          params = params || {};
> +          return new Promise((resolve, reject) => {
> +            this.runFindOperation(params, "HighlightSearchResults").then((result) => {
> +              switch (result) {
> +                case 1:

Please use String constants to clarify your signalling, like 'Success', 'OutOfRange' and 'NoResults'

::: toolkit/content/find-content.js
@@ +2,5 @@
> +const { Finder } = Cu.import("resource://gre/modules/Finder.jsm", {});
> +const { FinderHighlighter } = Cu.import("resource://gre/modules/FinderHighlighter.jsm", {});
> +const { FinderIterator } = Cu.import("resource://gre/modules/FinderIterator.jsm", {});
> +const _finder = new Finder(this.docShell);
> +const _iterator = Object.assign({}, FinderIterator);

Nice! This is exactly what I meant. I'm happy it's as simple as this in the end!

@@ +7,5 @@
> +const _highlighter = new FinderHighlighter(_finder);
> +
> +/**
> + * Native FinderIterator._collectFrames skips frames if they are scrolled out
> + * of viewport.  Override with method that doesn't do that.

Well, that's not how it should be :(

We're fixing this soon, but for the interim I guess it's fine to override it.

Comment 98

[u462496]

Great !  I'll jump on it right away.  Thanks for looking at that Mike.

The demos weren't up to date with the latest patches, so I think that may have been the problem.  I marked them obsolete.  I'll rebase with the latest MC and verify everything works.

Comment 99

[u462496]

Attachment: bug_1332144_API_P4_V13.diff

Update per comments.  Everything seems to work okay.  I'll upload a new demo when I have time.

Comment 100

[Matthew Wein [:mattw]]

Thanks, also don't forget to add the patch for the tests :)

Comment 101

[u462496]

(In reply to Matthew Wein [:mattw] from comment #100)
> Thanks, also don't forget to add the patch for the tests :)

Yes, the new patch made the patch for tests obsolete, so they need to be updated.

Comment 102

[u462496]

Maybe you fellows can help me out with a little glitch in getting the tests finished.  Now that we aren't returning a status code for success/failure from highlightResults, but instead are throwing an error, I am trying to test for the proper message being returned.  The code looks like this:

    try {
      await browser.find.highlightResults({ rangeIndex: 6 });
    } catch(e) {
      browser.test.assertEq("index supplied was out of range", e.message, "The error returns the proper message:");
    }

    data = await browser.find.search("xyz");
    try {
      await browser.find.highlightResults({ rangeIndex: 0 });
    } catch(e) {
      browser.test.assertEq("no search results to highlight", e.message, "The error returns the proper message:");
    }

However, the error message returned within the block is always "An unexpected error occurred".

The expected error messages do appear in the console however.

Comment 103

[u462496]

^^^^

Comment 104

[Tim Nguyen :ntim]

Comment on attachment 8886646 [details] [diff] [review]
bug_1332144_API_P4_V13.diff

Review of attachment 8886646 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/extensions/ext-find.js
@@ +1,1 @@
> +/* -*- Mode: indent-tabs-mode: nil; js-indent-level: 2 -*- */

As mentioned in comment 96, you need to register your new files (ext-find.js & find.json) inside their respective jar.mn files:

https://dxr.mozilla.org/mozilla-central/source/browser/components/extensions/jar.mn
https://dxr.mozilla.org/mozilla-central/source/browser/components/extensions/schemas/jar.mn

Without these changes, your tests will probably fail on mozilla-central, as ext-find.js and find.json won't be packaged.

Comment 105

[u462496]

Attachment: bug_1332144_unit_tests_P4_V2.diff

Don't know if there is much more I can do on the error thing, so I'm submitting the tests just testing to make sure an error gets thrown.  Let me know...

Comment 106

[Tim Nguyen :ntim]

(In reply to Kevin Jones from comment #102)
> Maybe you fellows can help me out with a little glitch in getting the tests
> finished.  Now that we aren't returning a status code for success/failure
> from highlightResults, but instead are throwing an error, I am trying to
> test for the proper message being returned.  The code looks like this:
> 
>     try {
>       await browser.find.highlightResults({ rangeIndex: 6 });
>     } catch(e) {
>       browser.test.assertEq("index supplied was out of range", e.message,
> "The error returns the proper message:");
>     }
> 
>     data = await browser.find.search("xyz");
>     try {
>       await browser.find.highlightResults({ rangeIndex: 0 });
>     } catch(e) {
>       browser.test.assertEq("no search results to highlight", e.message,
> "The error returns the proper message:");
>     }
> 
> However, the error message returned within the block is always "An
> unexpected error occurred".
> 
> The expected error messages do appear in the console however.

Not sure if it helps, but this is how the tabs API does it: https://dxr.mozilla.org/mozilla-central/source/browser/components/extensions/ext-tabs.js#337

https://dxr.mozilla.org/mozilla-central/source/browser/components/extensions/test/browser/browser_ext_tabs_cookieStoreId.js#60


It seems like you might need to do reject({ message: "your message" }) instead of reject("your message").

Does it help ?

Comment 107

[Matthew Wein [:mattw]]

You should also use Assert.rejects(...). Something like:

  await Assert.rejects(browser.find.highlightResults({rangeIndex: 0}),
                       /no search results to highlight/);

Comment 108

[u462496]

(In reply to Tim Nguyen :ntim from comment #104)
> Comment on attachment 8886646 [details] [diff] [review]
> bug_1332144_API_P4_V13.diff
> 
> Review of attachment 8886646 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/components/extensions/ext-find.js
> @@ +1,1 @@
> > +/* -*- Mode: indent-tabs-mode: nil; js-indent-level: 2 -*- */
> 
> As mentioned in comment 96, you need to register your new files (ext-find.js
> & find.json) inside their respective jar.mn files:
> 
> https://dxr.mozilla.org/mozilla-central/source/browser/components/extensions/
> jar.mn
> https://dxr.mozilla.org/mozilla-central/source/browser/components/extensions/
> schemas/jar.mn
> 
> Without these changes, your tests will probably fail on mozilla-central, as
> ext-find.js and find.json won't be packaged.

I can see I did that wrong, I should include them with the API patch and not the unit tests patch.

Comment 109

[u462496]

Attachment: bug_1332144_API_P4_V14.diff

Updated per Matt's suggestion for Promise rejection to be handled properly.

Comment 110

[u462496]

Attachment: bug_1332144_unit_tests_P4_V3.diff

Your suggestions worked perfectly Matt (with some modification).  Thanks for your help.

Comment 111

[u462496]

(In reply to Kevin Jones from comment #110)
> Created attachment 8886709 [details] [diff] [review]
> bug_1332144_unit_tests_P4_V3.diff
> 
> Your suggestions worked perfectly Matt (with some modification).  Thanks for
> your help.

er, and Tim...

Comment 112

[Mike de Boer [:mikedeboer]]

Comment on attachment 8886707 [details] [diff] [review]
bug_1332144_API_P4_V14.diff

Review of attachment 8886707 [details] [diff] [review]:
-----------------------------------------------------------------

Cool, nice progress! I'm curious to hear what your thoughts are on my comments below!
I think with those implemented we/you'll be super close to getting this landed ;-)

::: browser/components/extensions/ext-find.js
@@ +1,1 @@
> +/* -*- Mode: indent-tabs-mode: nil; js-indent-level: 2 -*- */

You're missing the MPL license header here.

@@ +14,5 @@
> +         *
> +         * @param {string} queryphrase - The string to search for.
> +         * @param {object} params optional - may contain any of the following properties,
> +         *   all of which are optional:
> +         *   {integer} tabId - Tab to query.  Defaults to the active tab.

nit: We don't really have integers in JS, be we do have 'number'. Can you change that in all the docblocks?

@@ +28,5 @@
> +         */
> +        search(queryphrase, params) {
> +          params = params || {};
> +          params.queryphrase = queryphrase;
> +          return new Promise(resolve => {

You don't need to add another promise in the mix here, you can simply do `return this.runFindOperation(params, "CollectSearchResults");`

@@ +29,5 @@
> +        search(queryphrase, params) {
> +          params = params || {};
> +          params.queryphrase = queryphrase;
> +          return new Promise(resolve => {
> +            this.runFindOperation(params, "CollectSearchResults").then((data) => {

I think it'd be good if we don't mix up the verbs 'find' and 'search' and use 'find' throughout instead. What do you think?

@@ +51,5 @@
> +        highlightResults(params) {
> +          params = params || {};
> +          return new Promise((resolve, reject) => {
> +            this.runFindOperation(params, "HighlightSearchResults").then((result) => {
> +              switch (result) {

I think it'd be best to move this logic to runFindOperation and add an operation 'result' to the other operations/ messages as well.
In that case you'll be able to do `return this.runFindOperation(params, "HighlightSearchResults");`, like above.

@@ +105,5 @@
> +        },
> +      },
> +    };
> +  }
> +}

nit: missing semicolon.

::: toolkit/content/find-content.js
@@ +1,1 @@
> +const { classes: Cc, interfaces: Ci, results: Cr, utils: Cu } = Components;

You're missing the MPL license header here and a `"strict mode";` declaration.

@@ +1,5 @@
> +const { classes: Cc, interfaces: Ci, results: Cr, utils: Cu } = Components;
> +const { Finder } = Cu.import("resource://gre/modules/Finder.jsm", {});
> +const { FinderHighlighter } = Cu.import("resource://gre/modules/FinderHighlighter.jsm", {});
> +const { FinderIterator } = Cu.import("resource://gre/modules/FinderIterator.jsm", {});
> +const _finder = new Finder(this.docShell);

What we generally see in frame scripts is a singleton, let's say 'const ExtFinderAPI = {};`, which allows you to namespace all your methods and variables so that name clashes are highly unlikely.

For example:
```js
var ExtFinder = {
  get finder() {
    if (!this._finder) {
      const { Finder } = Cu.import("resource://gre/modules/Finder.jsm", {});
      this._finder = Cu.import("resource://gre/modules/Finder.jsm", {}).Finder(docShell);
    }
    return this._finder;
  },

  get highlighter() {
    if (!this._highlighter) {
      const { FinderHighlighter } = Cu.import("resource://gre/modules/FinderHighlighter.jsm", {});
      this._highlighter = new FinderHighlighter(this.finder);
    }
    return this._highlighter;
  }

  init() {
    addMessageListener("ext-Finder:CollectSearchResults", this);
    // Same for other messages.
  },

  receiveMessage(message) {
    switch(message.name) {
      case "ext-Finder:CollectSearchResults":
        this.findRanges(message.data;
        break;
    }
  },

  async findRanges(options) {
    //...
  }
};

ExtFinder.init();
```

@@ +7,5 @@
> +const _highlighter = new FinderHighlighter(_finder);
> +
> +/**
> + * Native FinderIterator._collectFrames skips frames if they are scrolled out
> + * of viewport.  Override with method that doesn't do that.

This is also a bug in our current FinderIterator, no? I'd really like to see this fixed there.
If we can't fix it without skipping invisible (i)frames, we need to remove that feature just like you do here, because reliability trumps performance in this case.

Comment 113

[u462496]

(In reply to Mike de Boer [:mikedeboer] from comment #112)
> Comment on attachment 8886707 [details] [diff] [review]
> bug_1332144_API_P4_V14.diff
> 
> Review of attachment 8886707 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Cool, nice progress! I'm curious to hear what your thoughts are on my
> comments below!
> I think with those implemented we/you'll be super close to getting this
> landed ;-)
> 
> ::: browser/components/extensions/ext-find.js
> @@ +1,1 @@
> > +/* -*- Mode: indent-tabs-mode: nil; js-indent-level: 2 -*- */
> 
> You're missing the MPL license header here.

I don't know what you are saying.  The header appears the same as ext.tabs.js, ext.browser.js, ext.windows.js.  What license?

> I think it'd be best to move this logic to runFindOperation and add an
> operation 'result' to the other operations/ messages as well.
> In that case you'll be able to do `return this.runFindOperation(params,
> "HighlightSearchResults");`, like above.

Arggggg...  Wasn't it you, or someone else who asked to not return anything for highlightSearchResults and just throw assertions instead?  Maybe I musunderstood?

> ::: toolkit/content/find-content.js
> @@ +1,1 @@
> > +const { classes: Cc, interfaces: Ci, results: Cr, utils: Cu } = Components;
> 
> You're missing the MPL license header here and a `"strict mode";`
> declaration.

I still am not getting the license header thing, can you give an example of the text you want to see in there?

> This is also a bug in our current FinderIterator, no? I'd really like to see
> this fixed there.
> If we can't fix it without skipping invisible (i)frames, we need to remove
> that feature just like you do here, because reliability trumps performance
> in this case.

You said we could go ahead and ship this and fix it in FinderIterator later.  Did you change your mind on that?

Comment 114

[Matthew Wein [:mattw]]

Comment on attachment 8886709 [details] [diff] [review]
bug_1332144_unit_tests_P4_V3.diff

Review of attachment 8886709 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks!

Please run `./mach eslint` on these files and the ones from your other patch and fix the lint errors that it finds.
See https://wiki.mozilla.org/DevTools/CodingStandards

Comment 115

[u462496]

In addition to the comment above:

(In reply to Mike de Boer [:mikedeboer] from comment #112)
> @@ +1,5 @@
> > +const { classes: Cc, interfaces: Ci, results: Cr, utils: Cu } = Components;
> > +const { Finder } = Cu.import("resource://gre/modules/Finder.jsm", {});
> > +const { FinderHighlighter } = Cu.import("resource://gre/modules/FinderHighlighter.jsm", {});
> > +const { FinderIterator } = Cu.import("resource://gre/modules/FinderIterator.jsm", {});
> > +const _finder = new Finder(this.docShell);
> 
> What we generally see in frame scripts is a singleton, let's say 'const
> ExtFinderAPI = {};`, which allows you to namespace all your methods and
> variables so that name clashes are highly unlikely.
> 
> For example:
> ```js
> var ExtFinder = {
>   get finder() {
>     if (!this._finder) {
>       const { Finder } = Cu.import("resource://gre/modules/Finder.jsm", {});
>       this._finder = Cu.import("resource://gre/modules/Finder.jsm",
> {}).Finder(docShell);
>     }
>     return this._finder;
>   },
> 
>   get highlighter() {
>     if (!this._highlighter) {
>       const { FinderHighlighter } =
> Cu.import("resource://gre/modules/FinderHighlighter.jsm", {});
>       this._highlighter = new FinderHighlighter(this.finder);
>     }
>     return this._highlighter;
>   }
> 
>   init() {
>     addMessageListener("ext-Finder:CollectSearchResults", this);
>     // Same for other messages.
>   },
> 
>   receiveMessage(message) {
>     switch(message.name) {
>       case "ext-Finder:CollectSearchResults":
>         this.findRanges(message.data;
>         break;
>     }
>   },
> 
>   async findRanges(options) {
>     //...
>   }
> };
> 
> ExtFinder.init();
> ```

I think I see what you are talking about here.  However, the way this is written, won't this._finder and this._highlighter just be the imported jsm's?  Don't we want to create new instances of each?

Comment 116

[Mike de Boer [:mikedeboer]]

(In reply to Kevin Jones from comment #113)
> I don't know what you are saying.  The header appears the same as
> ext.tabs.js, ext.browser.js, ext.windows.js.  What license?

Hmmm, strange! Well, it certainly looks like the other files in the component don't contain license info, so perhaps it's some kind of new standard. I'll ask around privately, but please ignore my comment here then.

> Arggggg...  Wasn't it you, or someone else who asked to not return anything
> for highlightSearchResults and just throw assertions instead?  Maybe I
> musunderstood?

I doubt it was me, but perhaps... First off, I do sense your frustration here and I'm sorry to be the cause. I'm always looking for the most efficient and optimal implementation (note: _not_ perfect!) and this is just the way I see it. I think the change will be an improvement and worth considering.

> You said we could go ahead and ship this and fix it in FinderIterator later.
> Did you change your mind on that?

No, I didn't! It's indeed better to handle fixing the FinderIterator later. Sorry for the back-and-forth - unintentional.

(In reply to Kevin Jones from comment #115)
> I think I see what you are talking about here.  However, the way this is
> written, won't this._finder and this._highlighter just be the imported
> jsm's?  Don't we want to create new instances of each?

Yeah, I also see a mistake in the code there - but the example was mostly meant as illustration; a basic layout of how my idea expressed in code would look like. The lazy getters for `.finder` and `.highlighter` would and should not differ from the way you've implemented it in your current patch - those are correct!

As a final note: I'll be on vacation this and next week, so you won't be able to request review from me during that time. If you need and want a review in the meantime, please ask :jaws, :dao or :Gijs (in that order ;-) ). I think they're the most appropriate reviewers atm, even though they haven't touched findbar code in a long while...

Comment 117

[u462496]

(In reply to Mike de Boer [:mikedeboer] PTO until Aug 6th from comment #116)
> (In reply to Kevin Jones from comment #113)
> > I don't know what you are saying.  The header appears the same as
> > ext.tabs.js, ext.browser.js, ext.windows.js.  What license?
> 
> Hmmm, strange! Well, it certainly looks like the other files in the
> component don't contain license info, so perhaps it's some kind of new
> standard. I'll ask around privately, but please ignore my comment here then.
> 
> > Arggggg...  Wasn't it you, or someone else who asked to not return anything
> > for highlightSearchResults and just throw assertions instead?  Maybe I
> > musunderstood?
> 
> I doubt it was me, but perhaps... First off, I do sense your frustration
> here and I'm sorry to be the cause. I'm always looking for the most
> efficient and optimal implementation (note: _not_ perfect!) and this is just
> the way I see it. I think the change will be an improvement and worth
> considering.
> 
> > You said we could go ahead and ship this and fix it in FinderIterator later.
> > Did you change your mind on that?
> 
> No, I didn't! It's indeed better to handle fixing the FinderIterator later.
> Sorry for the back-and-forth - unintentional.
> 
> (In reply to Kevin Jones from comment #115)
> > I think I see what you are talking about here.  However, the way this is
> > written, won't this._finder and this._highlighter just be the imported
> > jsm's?  Don't we want to create new instances of each?
> 
> Yeah, I also see a mistake in the code there - but the example was mostly
> meant as illustration; a basic layout of how my idea expressed in code would
> look like. The lazy getters for `.finder` and `.highlighter` would and
> should not differ from the way you've implemented it in your current patch -
> those are correct!
> 
> As a final note: I'll be on vacation this and next week, so you won't be
> able to request review from me during that time. If you need and want a
> review in the meantime, please ask :jaws, :dao or :Gijs (in that order ;-)
> ). I think they're the most appropriate reviewers atm, even though they
> haven't touched findbar code in a long while...

Thank you for the explanations and clarification Mike.

As for reviews, I will just wait until you get back because my experience has been changing reviewers midstream just results in more review cycles and longer delays.  It seems like we are getting close and I don't want to throw that off.

Comment 118

[Tim Nguyen :ntim]

Just a little heads up since Mike is back :) Thanks for your awesome work!

Comment 119

[u462496]

Attachment: bug_1332144_API_P4_V16.diff

Updated per recommendations.  Thanks for the suggestions Mike, I think it's looking better.

I don't know if anyone else needs to look at these or who is available, please let me know.

Comment 120

[u462496]

Attachment: bug_1332144_unit_tests_P4_V16.diff

Unit tests updated for API updates.

Comment 121

[u462496]

Comment on attachment 8897017 [details] [diff] [review]
bug_1332144_API_P4_V16.diff

Please note that these changes make the demos on github obsolete.  I will try to update those when I have time.

Comment 122

[Mike de Boer [:mikedeboer]]

Comment on attachment 8897017 [details] [diff] [review]
bug_1332144_API_P4_V16.diff

Review of attachment 8897017 [details] [diff] [review]:
-----------------------------------------------------------------

Alright, ship it! I think anything else that's missing or might come up can and will be fixed in follow-ups. Thanks, Kevin!

::: browser/components/extensions/schemas/find.json
@@ +18,5 @@
> +    ]
> +  },
> +  {
> +    "namespace": "find",
> +    "description": "Use the <code>browser.find</code> API to interact with the browser's <code>Find</code> system.",

nit: s/system/interface/ ?

::: toolkit/content/find-content.js
@@ +95,5 @@
> +    this.iterator.reset();
> +
> +    // Cast `caseSensitive` and `entireWord` to boolean, otherwise _iterator.start will throw.
> +    let iteratorPromise = this.iterator.start({ word: queryphrase,
> +                                            caseSensitive: !!caseSensitive,

nit: Please format this object literal properly. I'd suggest:
```js
let iteratorPromise = this.iterator.start({
  word: queryPhrase,
  caseSensitive: !!caseSensitive,
  entireWord: !!entireWord,
  finder: this.finder,
  listener: this.finder,
});
```

@@ +254,5 @@
> +      status = "NoResults";
> +    }
> +    sendAsyncMessage("ext-Finder:HighlightResultsFinished", status);
> +  },
> +}

Missing semicolon.

Comment 123

[Mike de Boer [:mikedeboer]]

Comment on attachment 8897018 [details] [diff] [review]
bug_1332144_unit_tests_P4_V16.diff

Review of attachment 8897018 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM, but I'd like :aswan to do a final review before checkin' to see if the WebExtension-specific bits are up-to-snuff! Do you think that's a good idea, Kevin?

::: browser/components/extensions/test/browser/browser_ext_find.js
@@ +20,5 @@
> +  }
> +  getSelectedText();
> +}
> +
> +function waitForMessage(browser, topic) {

There's no generic WebExtension test method for this utility?

@@ +34,5 @@
> +add_task(async function testDuplicatePinnedTab() {
> +  async function background() {
> +    function awaitLoad(tabId) {
> +      return new Promise(resolve => {
> +        browser.tabs.onUpdated.addListener(function listener(tabId_, changed, tab) {

There's no helper method for this in the `browser.test` global?

Comment 124

[Mike de Boer [:mikedeboer]]

But perhaps MattW's r+ was already sufficient. I don't know.

Comment 125

[u462496]

(In reply to Mike de Boer [:mikedeboer] from comment #124)
> But perhaps MattW's r+ was already sufficient. I don't know.

Well, other than rebasing, fixing eslint errors and changing browser.find.search to browser.find.find, nothing has changed since he gave it r+.  But I'll let you be the judge.

Comment 126

[u462496]

(In reply to Mike de Boer [:mikedeboer] from comment #123)
> Comment on attachment 8897018 [details] [diff] [review]
> bug_1332144_unit_tests_P4_V16.diff
> 
> Review of attachment 8897018 [details] [diff] [review]:
> -----------------------------------------------------------------
> ::: browser/components/extensions/test/browser/browser_ext_find.js
> @@ +20,5 @@
> > +  }
> > +  getSelectedText();
> > +}
> > +
> > +function waitForMessage(browser, topic) {
> 
> There's no generic WebExtension test method for this utility?
> 
> @@ +34,5 @@
> > +add_task(async function testDuplicatePinnedTab() {
> > +  async function background() {
> > +    function awaitLoad(tabId) {
> > +      return new Promise(resolve => {
> > +        browser.tabs.onUpdated.addListener(function listener(tabId_, changed, tab) {
> 
> There's no helper method for this in the `browser.test` global?

I didn't find anything suitable for these.

Comment 127

[u462496]

IAE, it feels pretty solid to me.

Comment 128

[u462496]

Attachment: bug_1332144_API_P4_V17.diff

Updated with changes recommended by Mike.

Sorry for such a large patch, Mike, I was experimenting with MozReview (unsuccessfully) and didn't split up my commits.

If you want me to redo it, let me know.

Comment 129

[Mike de Boer [:mikedeboer]]

Comment on attachment 8897958 [details] [diff] [review]
bug_1332144_API_P4_V17.diff

Review of attachment 8897958 [details] [diff] [review]:
-----------------------------------------------------------------

No need to re-request review when I already r+'d your previous patches!

But now that I see your final patch: please try to invent a bit more elaborate commit message that tries to describe what this patch implements. Multi-line commit messages are allowed!

Comment 130

[u462496]

Attachment: bug_1332144_API_P4_V18.diff

Updated commit message.

Comment 131

[Tim Nguyen :ntim]

I would say the next step would be sending this through try then landing it :)

Comment 132

[u462496]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=cc362b0809302f11e26ac9ef3cb6544d47135dc8

Comment 133

[Tim Nguyen :ntim]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0c22f75cf2e69c076061208cc459fe2998a086e1

Comment 134

[u462496]

Attachment: bug_1332144_API_P4_V19.diff

Rebased, fixed eslint errors.

Comment 135

[u462496]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b7069ce748f0d6ef02626ea37b0d30c2df8b5304

Comment 136

[u462496]

I have uploaded current working demo WebExtensions on github:

https://github.com/Allasso/Find_API_demo_WE_basic
https://github.com/Allasso/Find_API_demo_WE_advanced

The basic version is good for getting started using the basic functionality of the API methods without the extra noise.

The advanced version demonstrates how an addon might utilize rangeData and rectData (returned from browser.find.find()) to:

  Display search results in context
  Implement customized highlighting of results.

See READMEs for more details.

Warning:  There is LOTs of detailed documentation within the code.

Comment 137

[u462496]

^^^^

Note there are xpi files in each which contain the complete addon.

Comment 138

[Tim Nguyen :ntim]

Landed on mozilla-inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/bb2763b51dad0705426714e9904d9b849e33aea8

Comment 139

[Tomislav Jovanovic :zombie]

I asked Phil to back this out:
https://hg.mozilla.org/integration/mozilla-inbound/rev/10b109b2b0af

because it's a new api that landed without a r+ from a webextension peer, and as Andrew noticed:
 - it adds a permission without a localization string (which means it is granted without asking the user)
 - it doesn't check hosts or the activeTab permission (which means it could affect any page, possibly even AMO)

Comment 140

[Tim Nguyen :ntim]

(In reply to Tomislav Jovanovic :zombie from comment #139)
>  - it doesn't check hosts or the activeTab permission (which means it could
> affect any page, possibly even AMO)

Not sure how this is an issue tbh, you're just finding stuff on a page, not really modifying the page or anything nasty.

Comment 141

[Mike de Boer [:mikedeboer]]

(In reply to Tomislav Jovanovic :zombie from comment #139)
> because it's a new api that landed without a r+ from a webextension peer,
> and as Andrew noticed:
>  - it adds a permission without a localization string (which means it is
> granted without asking the user)
>  - it doesn't check hosts or the activeTab permission (which means it could
> affect any page, possibly even AMO)

I do feel that this is a rather premature backout. It didn't even make m-c. Regardless, mwein was assigned to guide this bug to completion from the WebExtension side of things and granted r+ a long time ago. He's not a Mozilla employee anymore, until very recently, but I do believe he was a peer?
What's wrong with landing something that took a great while/ much effort/ considerable amounts of energy to finish and filing a bug afterwards to address possible concerns? It's 100% clear that we want this API in Firefox.

Comment 142

[Tim Nguyen :ntim]

> I do believe he was a peer?

From my conversation with :zombie on IRC, :mattw wasn't a WE peer: https://wiki.mozilla.org/index.php?title=WebExtensions/Reviews&oldid=1178898

Comment 143

[Tomislav Jovanovic :zombie]

Significant changes to WE code, especially those involving API design and permissions should really require a review by a WE peer.

(In reply to Mike de Boer [:mikedeboer] from comment #141)
> I do feel that this is a rather premature backout. It didn't even make m-c.
> Regardless, mwein was assigned to guide this bug to completion from the
> WebExtension side of things and granted r+ a long time ago. He's not a
> Mozilla employee anymore, until very recently, but I do believe he was a
> peer?

Backing out early is better than backing out late.  Matt isn't and wasn't a WE peer (being an employee is unrelated), and as far as I can see, a peer (Shane) only delegated feedback to Matt, not final review.

> What's wrong with landing something that took a great while/ much effort/
> considerable amounts of energy to finish and filing a bug afterwards to
> address possible concerns? It's 100% clear that we want this API in Firefox.

Amount of time/energy spent isn't what qualifies a patch for landing.  It looks like this patch violated basic security/permissions rules, which the Web Extensions Team considers significant enough to back out until it is reviewed.

Comment 144

[u462496]

(In reply to Tomislav Jovanovic :zombie from comment #143)
> It
> looks like this patch violated basic security/permissions rules, which the
> Web Extensions Team considers significant enough to back out until it is
> reviewed.

(In reply to Tomislav Jovanovic :zombie from comment #139)
> I asked Phil to back this out:
> https://hg.mozilla.org/integration/mozilla-inbound/rev/10b109b2b0af
> 
> because it's a new api that landed without a r+ from a webextension peer,
> and as Andrew noticed:
>  - it adds a permission without a localization string (which means it is
> granted without asking the user)
>  - it doesn't check hosts or the activeTab permission (which means it could
> affect any page, possibly even AMO)

If this is going to be backed out for these reasons, can someone please give more specific details on the permissions issues in question and what is needed for correction?

Comment 145

[Andrew Swan [:aswan]]

(In reply to Kevin Jones from comment #144)
> If this is going to be backed out for these reasons, can someone please give
> more specific details on the permissions issues in question and what is
> needed for correction?

With this patch as-is, any extension could trivially read the content of any web page without the user ever knowing or giving consent.  Permissions are documented on MDN:
https://developer.mozilla.org/en-US/Add-ons/WebExtensions/manifest.json/permissions

Without thinking about it in depth, I think a host permission and/or the activeTab permission would be appropriate for this API.  But the point of the backout is that this needs to be thought through carefully (and maybe reviewed by security?) before landing.

Comment 146

[u462496]

(In reply to Andrew Swan [:aswan] from comment #145)
> (In reply to Kevin Jones from comment #144)
> > If this is going to be backed out for these reasons, can someone please give
> > more specific details on the permissions issues in question and what is
> > needed for correction?
> 
> With this patch as-is, any extension could trivially read the content of any
> web page without the user ever knowing or giving consent.  Permissions are
> documented on MDN:
> https://developer.mozilla.org/en-US/Add-ons/WebExtensions/manifest.json/
> permissions
> 
> Without thinking about it in depth, I think a host permission and/or the
> activeTab permission would be appropriate for this API.  But the point of
> the backout is that this needs to be thought through carefully (and maybe
> reviewed by security?) before landing.

If I understand activeTab permission correctly (this api would only run on the active tab), this would severely cripple the purpose of this API, and defeat the purpose of the reason I requested the API in the first place.  The ultimate point of this was so that an addon (such as All Tabs Helper and Hugo) can search all tabs and return a list of results for all tabs.  This is why tabId is an optional parameter for browser.find.find().

<all_urls> permission would also be necessary as many of my users are searching documents loaded from the filesystem.  IIUC, <all_urls> would exclude resource and chrome urls which would be fine.

Comment 147

[u462496]

(In reply to Kevin Jones from comment #146) 
> If I understand activeTab permission correctly (this api would only run on
> the active tab), this would severely cripple the purpose of this API, and
> defeat the purpose of the reason I requested the API in the first place. 
> The ultimate point of this was so that an addon (such as All Tabs Helper and
> Hugo) can search all tabs and return a list of results for all tabs.  This
> is why tabId is an optional parameter for browser.find.find().

Perhaps `tabs` permission would be acceptable for this case?

Comment 148

[Tim Nguyen :ntim]

(In reply to Kevin Jones from comment #146)
> <all_urls> permission would also be necessary as many of my users are
> searching documents loaded from the filesystem.  IIUC, <all_urls> would
> exclude resource and chrome urls which would be fine.

It would also exclude mozilla.org websites as well unfortunately.

Comment 149

[u462496]

(In reply to Andrew Swan [:aswan] from comment #145)
> Permissions are
> documented on MDN:
> https://developer.mozilla.org/en-US/Add-ons/WebExtensions/manifest.json/
> permissions

The documentation here appears to suffice for explaining how to use permissions in an addon, but is not clear as to how they are set up in the API.

Comment 150

[Andrew Swan [:aswan]]

(In reply to Kevin Jones from comment #146)
> If I understand activeTab permission correctly (this api would only run on
> the active tab), this would severely cripple the purpose of this API, and
> defeat the purpose of the reason I requested the API in the first place. 
> The ultimate point of this was so that an addon (such as All Tabs Helper and
> Hugo) can search all tabs and return a list of results for all tabs.  This
> is why tabId is an optional parameter for browser.find.find().

I see.  In that case I think it makes sense to use the "find" permission (or perhaps some generalization of it?) with an appropriate localized string presented to the user when the permission is granted (at install time or when it is added as an optional permission).  Michelle Heusbusch or Scott Devaney can help with that.

> IIUC, <all_urls> would
> exclude resource and chrome urls which would be fine.

Without getting into the implementation, are you talking about things like about: urls?  Or literally about resource: and chrome: urls?  Regular users shouldn't even be seeing the last two.

(In reply to Kevin Jones from comment #147)
> Perhaps `tabs` permission would be acceptable for this case?

Lets not overload permissions with unrelated capabilities, it makes it very hard to explain them and for users to make an informed choice about whether to grant them.

(In reply to Tim Nguyen :ntim from comment #148)
> (In reply to Kevin Jones from comment #146)
> > <all_urls> permission would also be necessary as many of my users are
> > searching documents loaded from the filesystem.  IIUC, <all_urls> would
> > exclude resource and chrome urls which would be fine.
> 
> It would also exclude mozilla.org websites as well unfortunately.

No it would not.  Other applications of host permissions (eg webRequest) apply additional restrictions but those restrictions are not inherent to host permissions.

(In reply to Kevin Jones from comment #149)
> The documentation here appears to suffice for explaining how to use
> permissions in an addon, but is not clear as to how they are set up in the
> API.

Right, lets agree upon the desired behavior first, then I or somebody else can help you with the implementation.

Comment 151

[u462496]

(In reply to Andrew Swan [:aswan] from comment #150)
> (In reply to Kevin Jones from comment #146)
> > If I understand activeTab permission correctly (this api would only run on
> > the active tab), this would severely cripple the purpose of this API, and
> > defeat the purpose of the reason I requested the API in the first place. 
> > The ultimate point of this was so that an addon (such as All Tabs Helper and
> > Hugo) can search all tabs and return a list of results for all tabs.  This
> > is why tabId is an optional parameter for browser.find.find().
> 
> I see.  In that case I think it makes sense to use the "find" permission (or
> perhaps some generalization of it?) with an appropriate localized string
> presented to the user when the permission is granted (at install time or
> when it is added as an optional permission).  Michelle Heusbusch or Scott
> Devaney can help with that.

Can you provide their nicks?

> > IIUC, <all_urls> would
> > exclude resource and chrome urls which would be fine.
> 
> Without getting into the implementation, are you talking about things like
> about: urls?  Or literally about resource: and chrome: urls?  Regular users
> shouldn't even be seeing the last two.

What I am essentially saying is that <all_urls> (which matches "http", "https", "file", "ftp", "app") would be sufficient for my needs.  Even just the first 3 would be fine.  I don't need about: urls.

> (In reply to Kevin Jones from comment #149)
> > The documentation here appears to suffice for explaining how to use
> > permissions in an addon, but is not clear as to how they are set up in the
> > API.
> 
> Right, lets agree upon the desired behavior first, then I or somebody else
> can help you with the implementation.

So I think (hope) I've made clear my needs:
  Search all tabs with at least http://, https:// and file:// urls.

And it appears to me that generally I can accomplish this while satisfying policy by:
   <all_urls> permissions
   "file" permission with an appropriate localized string
   
I just need to know how to get there.  If this all looks good to you I will commence with seeking input from Michelle or Scott.

Please let me know if I can continue in this vein.

Comment 152

[u462496]

(In reply to Kevin Jones from comment #151)
> So I think (hope) I've made clear my needs:
>   Search all tabs with at least http://, https:// and file:// urls.

The more I think about it, it may be good to include "ftp" and "app" as well, so yes, <all_urls>

Comment 153

[Andrew Swan [:aswan]]

(In reply to Kevin Jones from comment #151)
> > I see.  In that case I think it makes sense to use the "find" permission (or
> > perhaps some generalization of it?) with an appropriate localized string
> > presented to the user when the permission is granted (at install time or
> > when it is added as an optional permission).  Michelle Heusbusch or Scott
> > Devaney can help with that.
> 
> Can you provide their nicks?

Its probably easiest to just needinfo them on bugzilla where they are mheubusch and sdevaney both @mozilla.com

> And it appears to me that generally I can accomplish this while satisfying
> policy by:
>    <all_urls> permissions
>    "file" permission with an appropriate localized string

I assume you meant "find" not "file".  With a suitable permission string I think that named permission would be sufficient (ie, no need for both that and host permissions).  While we're here, can you please also get the webextensions portion of the patch reviewed by one of {mixedpuppy,zombie,rpl}?

Comment 154

[u462496]

Comment on attachment 8898845 [details] [diff] [review]
bug_1332144_API_P4_V19.diff

Hello Michelle and Scott,

Andrew suggested I ask for help from one of you to properly set up permission and provide an appropriate localization string for my current patch.  The string I would like to use for my permission is "find".

(See the first part of comment 150 and comment 153 - and yes, I meant "find", not "file".)

Comment 155

[Andrew Swan [:aswan]]

Sorry I guess I was unclear -- the localized string I was referring to is the one that we actually show to users that explains what an extension can do if it is granted this permission.  In this case, I think it would be something like:
"Read the contents of all web pages"

Comment 156

[u462496]

(In reply to Andrew Swan [:aswan] from comment #155)
> Sorry I guess I was unclear -- the localized string I was referring to is
> the one that we actually show to users that explains what an extension can
> do if it is granted this permission.  In this case, I think it would be
> something like:
> "Read the contents of all web pages"

That is what I thought you meant, so I'm probably the one who was unclear.

From what I have gathered, it seems to me the only thing the patch needs at this point is something like:

+webextPerms.description.find=Read the contents of all web pages

in browser/locales/en-US/chrome/browser/browser.properties.

So is it the exact wording that I am needing Michelle or Scott for?

Comment 157

[Andrew Swan [:aswan]]

Correct, and they need as input a description of what exactly is allowed when this permission is granted.

Comment 158

[u462496]

(In reply to Kevin Jones from comment #154)
> Comment on attachment 8898845 [details] [diff] [review]
> bug_1332144_API_P4_V19.diff
> 
> Hello Michelle and Scott,
> 
> Andrew suggested I ask for help from one of you to properly set up
> permission and provide an appropriate localization string for my current
> patch.  The string I would like to use for my permission is "find".
> 
> (See the first part of comment 150 and comment 153 - and yes, I meant
> "find", not "file".)

(In reply to Andrew Swan [:aswan] from comment #157)
> Correct, and they need as input a description of what exactly is allowed
> when this permission is granted.

Michelle/Scott:

Exactly what I need is this:

To be able to search all tabs (active or not) for text within the web page, in the same way native Findbar does.  So this API would effectively be able to read all text within the web page.

Comment 159

[sdevaney]

I'd propose this string: 

Read the Web page text of all open tabs.

Comment 160

[u462496]

(In reply to sdevaney from comment #159)
> I'd propose this string: 
> 
> Read the Web page text of all open tabs.

Great, thanks.

Comment 161

[u462496]

Attachment: bug_1332144_API_P5_V1.diff

Rebased,added localization string for `find` permission (browser.properties).

Shane, you may be aware of the situation with this bug by now, I'm asking you to fulfill the WE peer review upon recommendation.

mattw already gave this his r+ (less the localization string addition).

Thanks for helping to get this thing landed!

Comment 162

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8898845 [details] [diff] [review]
bug_1332144_API_P4_V19.diff

This looks great, but a couple things need to be addressed.

loadedFrameScripts is never pruned, and I'm not certain I like how the framescript is being added.  It looks like the framescript would also be loaded for each context (extension) calling find.  That needs to be figured out.  

I think that find-content.js should be a module that is lazy loaded in toolkit/content/browser-content.js.  A small class in browser-content that just listens for/handles/passes on the messages to a findContent.jsm that is lazy loaded wouldn't add any real overhead.  Then the find api would never need to consider if the framescript is loaded.

Mike, what do you think about that?

runFindOperation isn't an API call, but it's being returned in getAPI.  Move it out of that object, it can just be a separate function.

Also, please document the result structures (eg. rectData and rangeData) in a comment for the dev docs (though I wonder if they'll find it through all the comments).

Comment 163

[u462496]

Attachment: dev_doc.txt

Dev Doc helps.

Comment 164

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8898845 [details] [diff] [review]
bug_1332144_API_P4_V19.diff

Slightly more formal review response.  Lets just go ahead with this, I don't see any reason not to.

>+this.find = class extends ExtensionAPI {
>+  getAPI(context) {
>+    let loadedFrameScripts = {};

Lets remove loadedFrameScripts and the frameScript load below.

>+        runFindOperation(params, message) {

move to a top level function, not part of getAPI results.

>+          let {tabId} = params;
>+          let tab = tabId ? tabTracker.getTab(tabId) : tabTracker.activeTab;
>+          let browser = tab.linkedBrowser;
>+          let mm = browser.messageManager;
>+          tabId = tabId || tabTracker.getId(tab);
>+
>+          if (!loadedFrameScripts[tabId]) {
>+            mm.loadFrameScript("chrome://global/content/find-content.js", false, true);
>+            loadedFrameScripts[tabId] = true;
>+          }

Drop the frame script loading.


>diff --git a/toolkit/content/find-content.js b/toolkit/content/find-content.js
>new file mode 100644
>--- /dev/null
>+++ b/toolkit/content/find-content.js

Move this file to toolkit/components/extensions/FindContent.jsm

>+  init(docShell) {
>+    this._docShell = docShell;

docShell is a global in frame scripts, not sure why you need to hold onto it.

>+    addMessageListener("ext-Finder:CollectResults", this);
>+    addMessageListener("ext-Finder:HighlightResults", this);
>+    addMessageListener("ext-Finder:clearHighlighting", this);
>+  },
>+
>+  receiveMessage(message) {
>+    switch (message.name) {
>+      case "ext-Finder:CollectResults":
>+        this.findRanges(message.data);
>+        break;
>+      case "ext-Finder:HighlightResults":
>+        this.highlightResults(message.data);
>+        break;
>+      case "ext-Finder:clearHighlighting":
>+        this.highlighter.highlight(false);
>+        break;
>+    }
>+  },

Move the message listener into a class in toolkit/content/browser-content.js and lazy import our find code (e.g. ReaderMode).

Comment 165

[u462496]

Attachment: bug_1332144_API_P5_V2.diff

Updated per comments.

Comment 166

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8900562 [details] [diff] [review]
bug_1332144_API_P5_V2.diff


>diff --git a/browser/components/extensions/ext-find.js b/browser/components/extensions/ext-find.js
>new file mode 100644
>--- /dev/null
>+++ b/browser/components/extensions/ext-find.js

>+const runFindOperation = function runFindOperation(params, message) {

Drop "const runFindOperation = "

>diff --git a/toolkit/components/extensions/FindContent.jsm b/toolkit/components/extensions/FindContent.jsm
>new file mode 100644
>--- /dev/null
>+++ b/toolkit/components/extensions/FindContent.jsm
>@@ -0,0 +1,254 @@
>+/* This Source Code Form is subject to the terms of the Mozilla Public
>+ * License, v. 2.0. If a copy of the MPL was not distributed with this
>+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>+
>+"use strict";
>+
>+this.EXPORTED_SYMBOLS = ["FindContent"];
>+
>+/* exported FindContent */
>+
>+const {classes: Cc, interfaces: Ci, results: Cr, utils: Cu} = Components;
>+
>+var FindContent = {
>+  finder: null,
>+  _iterator: null,
>+  _highlighter: null,

I'd like the jsm to be stateless.  Can this be a class (like Finder.jsm)? We can keep a reference to a FinderContent instance in ExtFind.

Comment 167

[u462496]

Attachment: bug_1332144_API_P5_V3.diff

Update per comments.  Thanks Shane...

Comment 168

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8900958 [details] [diff] [review]
bug_1332144_API_P5_V3.diff


>+++ b/browser/components/extensions/ext-find.js
      data received by the message listener.
>+ */
>+const runFindOperation = function runFindOperation(params, message) {

"const runFindOperation = " shouldn't be necessary.

>+++ b/toolkit/components/extensions/FindContent.jsm
>@@ -0,0 +1,252 @@
>+/* This Source Code Form is subject to the terms of the Mozilla Public
>+ * License, v. 2.0. If a copy of the MPL was not distributed with this
>+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>+
>+"use strict";
>+
>+this.EXPORTED_SYMBOLS = ["FindContent"];
>+
>+/* exported FindContent */
>+
>+const {classes: Cc, interfaces: Ci, results: Cr, utils: Cu} = Components;
>+
>+function FindContent(docShell) {
>+  const {Finder} = Cu.import("resource://gre/modules/Finder.jsm", {});
>+  this.finder = new Finder(docShell);
>+}
>+
>+FindContent.prototype = {

Can you use the class syntax instead of prototype?

That should wrap it up, r+ with those changes, and run it through try.

Comment 169

[u462496]

(In reply to Shane Caraveo (:mixedpuppy) from comment #168)
> Comment on attachment 8900958 [details] [diff] [review]
> bug_1332144_API_P5_V3.diff
> 
> 
> >+++ b/browser/components/extensions/ext-find.js
>       data received by the message listener.

I don't understand this comment.

Comment 170

[u462496]

I am getting this eslint error:

./mach eslint browser/components/extensions/test/browser/browser_ext_find.js
/home/allasso/mozilla-central/mozilla-central_API_Find/browser/components/extensions/test/browser/browser_ext_find.js
  6:22  warning  content is a possible Cross Process Object Wrapper (CPOW).  mozilla/no-cpows-in-tests (eslint)

??

(In reply to Kevin Jones from comment #169)
> (In reply to Shane Caraveo (:mixedpuppy) from comment #168)
> > Comment on attachment 8900958 [details] [diff] [review]
> > bug_1332144_API_P5_V3.diff
> > 
> > 
> > >+++ b/browser/components/extensions/ext-find.js
> >       data received by the message listener.
> 
> I don't understand this comment.

I see, you were referring to JSDoc, right?

Comment 171

[u462496]

^^^^

That is, warning.

Comment 172

[u462496]

Attachment: bug_1332144_API_P5_V4.diff

Updated per comments.

Comment 173

[u462496]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=152d572bc95a614d4a3a80897315c6fe891f31d9

Comment 174

[u462496]

Attachment: dev_doc_2.txt

Updated dev doc and corrected errors.

Comment 175

[u462496]

(In reply to Kevin Jones from comment #173)
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=152d572bc95a614d4a3a80897315c6fe891f31d9

I don't see any failures resulting from the patch.  If it looks okay to you, Shane, I'll go ahead and request checkin.

Comment 176

[Shane Caraveo (:mixedpuppy)]

Lets land this!

Comment 177

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a2241a469dbe
Add browser.find extension API. r=mikedeboer, r=mixedpuppy

Comment 178

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/29beef40b82d
Fix no-extra-semi ESLint failure in ext-find.js. r=me

Comment 179

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/a2241a469dbe
https://hg.mozilla.org/mozilla-central/rev/29beef40b82d

Comment 180

[Francesco Lodolo [:flod]]

(In reply to sdevaney from comment #159)
> I'd propose this string: 
> 
> Read the Web page text of all open tabs.

Isn't web supposed to be lowercase?
http://design.firefox.com/photon/copy/word-list.html#w

Also, why "web page text" and not just "text"? They can only read the text displayed on the page, not the code?

Comment 181

[Francesco Lodolo [:flod]]

(In reply to Francesco Lodolo [:flod] from comment #180)
> Also, why "web page text" and not just "text"? They can only read the text
> displayed on the page, not the code?

I should have read the bug commit, it's basically access to the "Find" function, so the answer is yes. 

Question about "Web" remain. Personally I find "web page text" very hard to grasp.

Comment 182

[Shane Caraveo (:mixedpuppy)]

see Francesco's comments above.

Comment 183

[sdevaney]

Ah. Thanks for the photon style guide reference re: "web" (it has historically been capitalized on AMO but let's defer to photon style for this). And after reflection I agree the wording "web page text" is awkward an arguably unnecessary. Let's just go with...

Read the text of all open tabs.

Comment 184

[Francesco Lodolo [:flod]]

This string hasn't been exposed for localization tools yet, but I can't hold on much longer. Once it's exposed, you need a new string ID to change the content.
https://developer.mozilla.org/en-US/docs/Mozilla/Localization/Localization_content_best_practices#Changing_existing_strings

Questions:
* How easy is to change the string ID? If it's hard, extra attention should go into reviewing strings before they land, and likely another pair of eyes on review.
* Can we get a string only patch landed as soon as possible?

Comment 185

[Francesco Lodolo [:flod]]

(In reply to Francesco Lodolo [:flod] from comment #184)
> * Can we get a string only patch landed as soon as possible?

Splitting up in a follow up bug. The other question remains, leaving NI for that.

Comment 186

[Tomislav Jovanovic :zombie]

(In reply to Francesco Lodolo [:flod] from comment #184)
> Questions:
> * How easy is to change the string ID? If it's hard, extra attention should
> go into reviewing strings before they land, and likely another pair of eyes
> on review.

Filed bug 1394936, so clearing NIs here.

Comment 187

[u462496]

Attachment: dev_doc_3.txt

Clarified the structure and use of rectData.  Provided github links for demo webextensions.

Comment 188

[Will Bamberg [:wbamberg]]

Thanks for the dev-doc!

I'm working on some docs for the find API, and would love it if you could help me understand how to use rangeData.

I've got an extension that does this:

  browser.browserAction.onClicked.addListener((tab) => {

    function logContext(matches) {
      console.log(matches);
    }

    browser.find.find("examples", {includeRangeData: true}).then(logContext);

  });

I load https://example.org and press the button. I see 2 matches, as I would expect, but the rangeData is not what I would expect:

  [0]:
    startTextNodePos: 13
    endTextNodePos: 13
    startOffset: 56
    endOffset: 64

  [1]:
    startTextNodePos: 17
    endTextNodePos: 17
    startOffset: 110
    endOffset: 118

The offset values are what I would expect, but I would have expected the text node positions to be the same for both matches (13) since both matches are in the same node.

Comment 189

[u462496]

(In reply to Will Bamberg [:wbamberg] from comment #188)
> Thanks for the dev-doc!
> 
> I'm working on some docs for the find API, and would love it if you could
> help me understand how to use rangeData.
> 
> I've got an extension that does this:
> 
>   browser.browserAction.onClicked.addListener((tab) => {
> 
>     function logContext(matches) {
>       console.log(matches);
>     }
> 
>     browser.find.find("examples", {includeRangeData: true}).then(logContext);
> 
>   });
> 
> I load https://example.org and press the button. I see 2 matches, as I would
> expect, but the rangeData is not what I would expect:
> 
>   [0]:
>     startTextNodePos: 13
>     endTextNodePos: 13
>     startOffset: 56
>     endOffset: 64
> 
>   [1]:
>     startTextNodePos: 17
>     endTextNodePos: 17
>     startOffset: 110
>     endOffset: 118
> 
> The offset values are what I would expect, but I would have expected the
> text node positions to be the same for both matches (13) since both matches
> are in the same node.

Can you supply the code for your extension?

Comment 190

[Will Bamberg [:wbamberg]]

Attachment: find-example.zip

Comment 191

[Will Bamberg [:wbamberg]]

> Can you supply the code for your extension?

I've attached it to this bug.

Comment 192

[u462496]

Thanks Will, you've uncovered a small although nefarious bug.  I've provided a patch in bug 1406912.

Comment 193

[Will Bamberg [:wbamberg]]

I've written some docs for this API: https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/find

Most of the meat is in find.find(), as you'd expect. I'd love it if you could take a look.

I thought it might be more helpful if the API split the data in rangeData by frames at the top level, because AFAICT most of the time, if you are using rangeData, you'll be using it in a content script, and a content script's environment is a specific frame. So the background script is going to have to do this work of splitting up rangeData by frame, and passing each section into a different script. Unless I'm missing something (the example in the dev-doc seems to assume that a content script is global across all frames, but I don't think it is).

I also wondered whether the framePos in rangeData matches up with the frameIds in the tabs.executeScript API. I didn't check this. But it would be good if they did.

Comment 194

[u462496]

(In reply to Will Bamberg [:wbamberg] from comment #193)
> I've written some docs for this API:
> https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/find
> 
> Most of the meat is in find.find(), as you'd expect. I'd love it if you
> could take a look.
> 
> I thought it might be more helpful if the API split the data in rangeData by
> frames at the top level, because AFAICT most of the time, if you are using
> rangeData, you'll be using it in a content script, and a content script's
> environment is a specific frame. So the background script is going to have
> to do this work of splitting up rangeData by frame, and passing each section
> into a different script. Unless I'm missing something (the example in the
> dev-doc seems to assume that a content script is global across all frames,
> but I don't think it is).
> 
> I also wondered whether the framePos in rangeData matches up with the
> frameIds in the tabs.executeScript API. I didn't check this. But it would be
> good if they did.

Yes, I'll take a look.

I think your suggestions for the API might be good, though I wouldn't have time to invest in making those changes, and it could be some time before I do.

Comment 195

[u462496]

(In reply to Will Bamberg [:wbamberg] from comment #193)
> I've written some docs for this API:
> https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/find
> 
> Most of the meat is in find.find(), as you'd expect. I'd love it if you
> could take a look.

Thanks a lot Will, the docs look excellent, very thorough.  Everything also appears to be accurate to me.  The examples look very good also, although I didn't analyze them too carefully, so I trust they have been tested.

Thanks for all your hard work on this.

Comment 196

[Victor Carciu]

Is manual testing required on this bug? If Yes, please provide some STR and the proper webextension(if required), if No set the “qe-verify-“ flag.