1004515 - [e10s] Implement find occurrence counter for RemoteFinder.jsm

Status: RESOLVED FIXED

(Toolkit :: Find Toolbar, defect)

Description

[Mike de Boer [:mikedeboer]]

Bug 257061 implements a counter that displays the number of found items AND shows which occurrence is currently selected.

This has not been implemented/ enabled for the e10s version (RemoteFinder.jsm).
My current assessment is that this shouldn't be hard, mainly adding one or two message passing routes.

Comment 1

[Blake Kaplan (:mrbkap) (inactive)]

Giving to Alex to work on. He should have a patch shortly!

Comment 2

[atifrea]

Attachment: Fixed the bug by adding another message for child-to-parent communication. r=felipe

Comment 3

[atifrea]

Comment on attachment 8446219 [details] [diff] [review]
Fixed the bug by adding another message for child-to-parent communication. r=felipe

># HG changeset patch
># User Alexandru Tifrea <[e-mail]>
>
>Bug 1004515 - Fixed the bug by adding another message for child-to-parent communication. r=mikedeboer
>
>diff --git a/toolkit/modules/Finder.jsm b/toolkit/modules/Finder.jsm
>index d6562bc..10b4b0f 100644
>--- a/toolkit/modules/Finder.jsm
>+++ b/toolkit/modules/Finder.jsm
>@@ -7,18 +7,16 @@ this.EXPORTED_SYMBOLS = ["Finder"];
> const Ci = Components.interfaces;
> const Cc = Components.classes;
> const Cu = Components.utils;
> 
> Cu.import("resource://gre/modules/XPCOMUtils.jsm");
> Cu.import("resource://gre/modules/Geometry.jsm");
> Cu.import("resource://gre/modules/Services.jsm");
> 
>-let console = (Cu.import("resource://gre/modules/devtools/Console.jsm", {})).console;
>-
> XPCOMUtils.defineLazyServiceGetter(this, "TextToSubURIService",
>                                          "@mozilla.org/intl/texttosuburi;1",
>                                          "nsITextToSubURI");
> XPCOMUtils.defineLazyServiceGetter(this, "Clipboard",
>                                          "@mozilla.org/widget/clipboard;1",
>                                          "nsIClipboard");
> XPCOMUtils.defineLazyServiceGetter(this, "ClipboardHelper",
>                                          "@mozilla.org/widget/clipboardhelper;1",
>@@ -273,24 +271,21 @@ Finder.prototype = {
>       this._countMatchesInWindow(aWord, aMatchLimit, aLinksOnly, frame, result);
>     }
> 
>     // The `_currentFound` and `_framesToCount` properties are only used for
>     // internal bookkeeping between recursive calls.
>     delete result._currentFound;
>     delete result._framesToCount;
> 
>-    console.log("Sunt in Finder");
>-    console.log(result);
>-    //for (let l of this._listeners) {
>-    //  try {
>-    //    l.onMatchesCountResult(result);
>-    //  } catch (ex) {}
>-    //}
>-    this._notify(aWord, result, false, false, false);
>+    for (let l of this._listeners) {
>+      try {
>+        l.onMatchesCountResult(result);
>+      } catch (ex) {}
>+    }
>   },
> 
>   /**
>    * Counts the number of matches for the searched word in the passed window's
>    * content.
>    * @param aWord
>    *        the word to search for.
>    * @param aMatchLimit
>diff --git a/toolkit/modules/RemoteFinder.jsm b/toolkit/modules/RemoteFinder.jsm
>index b19c94b..b574480 100644
>--- a/toolkit/modules/RemoteFinder.jsm
>+++ b/toolkit/modules/RemoteFinder.jsm
>@@ -6,45 +6,47 @@
> this.EXPORTED_SYMBOLS = ["RemoteFinder", "RemoteFinderListener"];
> 
> const Ci = Components.interfaces;
> const Cc = Components.classes;
> const Cu = Components.utils;
> 
> Cu.import("resource://gre/modules/XPCOMUtils.jsm");
> 
>-let console = (Cu.import("resource://gre/modules/devtools/Console.jsm", {})).console;
>-
> function RemoteFinder(browser) {
>   this._browser = browser;
>   this._listeners = [];
>   this._searchString = null;
> 
>   this._browser.messageManager.addMessageListener("Finder:Result", this);
>+  this._browser.messageManager.addMessageListener("Finder:MatchesResult", this);
>   this._browser.messageManager.sendAsyncMessage("Finder:Initialize");
> }
> 
> RemoteFinder.prototype = {
>   addResultListener: function (aListener) {
>     if (this._listeners.indexOf(aListener) === -1)
>       this._listeners.push(aListener);
>   },
> 
>   removeResultListener: function (aListener) {
>     this._listeners = this._listeners.filter(l => l != aListener);
>   },
> 
>   receiveMessage: function (aMessage) {
>     this._searchString = aMessage.data.searchString;
> 
>-    console.log("AM PRIMIT UN MESAJ IN COPIL");
>-    console.log(aMessage.data);
>-
>+    // the parent can receive either one the two the types of messages
>     for (let l of this._listeners) {
>-      l.onFindResult(aMessage.data);
>+        if (aMessage.name == "Finder:Result") {
>+          l.onFindResult(aMessage.data);
>+        }
>+        if (aMessage.name == "Finder:MatchesResult") {
>+          l.onMatchesCountResult(aMessage.data);
>+        }
>     }
>   },
> 
>   get searchString() {
>     return this._searchString;
>   },
> 
>   set caseSensitive(aSensitive) {
>@@ -97,18 +99,16 @@ RemoteFinder.prototype = {
> }
> 
> function RemoteFinderListener(global) {
>   let {Finder} = Cu.import("resource://gre/modules/Finder.jsm", {});
>   this._finder = new Finder(global.docShell);
>   this._finder.addResultListener(this);
>   this._global = global;
> 
>-  console.log("CEVA..ORICE");
>-
>   for (let msg of this.MESSAGES) {
>     global.addMessageListener(msg, this);
>   }
> }
> 
> RemoteFinderListener.prototype = {
>   MESSAGES: [
>     "Finder:CaseSensitive",
>@@ -118,21 +118,26 @@ RemoteFinderListener.prototype = {
>     "Finder:EnableSelection",
>     "Finder:RemoveSelection",
>     "Finder:FocusContent",
>     "Finder:KeyPress",
>     "Finder:MatchesCount"
>   ],
> 
>   onFindResult: function (aData) {
>-    console.log("Asta e ce trimit catre child");
>-    console.log(aData);
>+      // the parent can receive either one the two the types of messages
>     this._global.sendAsyncMessage("Finder:Result", aData);
>   },
> 
>+  // receives messages with results of requestMatchesCount
>+  // and passes them forward to the parent
>+  onMatchesCountResult: function (aData) {
>+    this._global.sendAsyncMessage("Finder:MatchesResult", aData);
>+  },
>+
>   //XXXmikedeboer-20131016: implement |shouldFocusContent| here to mitigate
>   //                        issues like bug 921338 and bug 921308.
>   shouldFocusContent: function () {
>     return true;
>   },
> 
>   receiveMessage: function (aMessage) {
>     let data = aMessage.data;
>@@ -162,18 +167,13 @@ RemoteFinderListener.prototype = {
>         this._finder.focusContent();
>         break;
> 
>       case "Finder:KeyPress":
>         this._finder.keyPress(data);
>         break;
> 
>       case "Finder:MatchesCount":
>-        console.log("Aici afisez ceva");
>-        console.log(this._finder);
>-        console.log(data.searchString);
>-        console.log(data.matchLimit);
>-        console.log(data.linksOnly);
>         this._finder.requestMatchesCount(data.searchString, data.matchLimit, data.linksOnly);
>         break;
>     }
>   }
> };

Comment 4

[Mike de Boer [:mikedeboer]]

Hi Alexandru, thanks for working on this!

I'm afraid you uploaded the wrong patch, because the differences are compared to a state that you used for debugging I think.

The view we use to review patches is https://bugzilla.mozilla.org/page.cgi?id=splinter.html&bug=1004515&attachment=8446219 and I need to get all the information from there to see the work that you've done.

Can you upload a new patch that correctly diffs between mozilla-central (or inbound) and your patch? If you don't know how to do that exactly, you can ask me anytime on IRC in #fx-team. You can ping me (mikedeboer) or just ask your question(s) directly.

I also recommend to use a more descriptive commit message, like `Bug 1004515: add support for the find counter in RemoteFinder.jsm. r=mikedeboer` or something similar.

Comment 5

[Mike de Boer [:mikedeboer]]

Comment on attachment 8446219 [details] [diff] [review]
Fixed the bug by adding another message for child-to-parent communication. r=felipe

As a side note, you don't really need to paste an updated patch in a comment when you upload your next version.

Comment 6

[atifrea]

Attachment: Added the code for the reply message from parent to child

Comment 7

[atifrea]

Attachment: Bug 1004515 - Patch v2: add support for the find counter in RemoteFinder.jsm. r=mikedeboer

Comment 8

[Mike de Boer [:mikedeboer]]

Comment on attachment 8446664 [details] [diff] [review]
Bug 1004515 - Patch v2: add support for the find counter in RemoteFinder.jsm. r=mikedeboer

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

Next time I'll just r+ this patch, because I couldn't find anything but a few nits.

However, just for this one I'd like to see the patch in its entirety with my comments addressed before we land this.

Looks like a job well done! Thank you. :)

::: toolkit/modules/RemoteFinder.jsm
@@ -1,1 @@
> -// -*- Mode: javascript; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*-

I don't understand how this ended up on this side of the patch... it's already set to value on the right :/

Please just discard this change.

@@ +34,4 @@
>    receiveMessage: function (aMessage) {
>      this._searchString = aMessage.data.searchString;
>  
> +    // the parent can receive either one the two the types of messages

Nit: We usually write comments as complete, correct English sentences; _with_ an uppercase sentences' first character and punctuation (a dot at the end of each sentence.)

@@ +38,5 @@
>      for (let l of this._listeners) {
> +        if (aMessage.name == "Finder:Result") {
> +          l.onFindResult(aMessage.data);
> +        }
> +        if (aMessage.name == "Finder:MatchesResult") {

Please make this an `else if`

@@ +123,4 @@
>    ],
>  
>    onFindResult: function (aData) {
> +      // the parent can receive either one the two the types of messages

Nit: same as before, and also the indentation seems wrong here.

@@ +127,5 @@
>      this._global.sendAsyncMessage("Finder:Result", aData);
>    },
>  
> +  // receives messages with results of requestMatchesCount
> +  // and passes them forward to the parent

Nit: please correct this comment similarly.

Comment 9

[atifrea]

Attachment: Patch v3: add support for the find counter in RemoteFinder.jsm. r=mikedeboer

Comment 10

[atifrea]

Attachment: Bug 1004515 - Patch v4: add support for the find counter in RemoteFinder.jsm. r=mikedeboer

Comment 11

[atifrea]

Attachment: Bug 1004515 - Patch v4: add support for the find counter in RemoteFinder.jsm. r=mikedeboer

Comment 12

[atifrea]

Attachment: bug1004515patch_v4.diff

Comment 13

[Mike de Boer [:mikedeboer]]

Comment on attachment 8447814 [details] [diff] [review]
bug1004515patch_v4.diff

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

Oops! This is the wrong diff/ patch.

Please post a correct patch that diffs between mozilla-central (or inbound) and your patch.

It looks like you made the changes I asked for on top of the work you did before and didn't squash the commits after your work was completed. So in effect, you posted an interdiff here, instead of the full patch.

Looking forward to seeing the next version!

Comment 14

[atifrea]

Attachment: Patch v4: add support for the find counter in RemoteFinder.jsm. r=mikedeboer

Comment 15

[atifrea]

Attachment: Bug 1004515 - Patch v5: add support for the find counter in RemoteFinder.jsm. r=mikedeboer

Comment 16

[Mike de Boer [:mikedeboer]]

Comment on attachment 8448091 [details] [diff] [review]
Bug 1004515 - Patch v5: add support for the find counter in RemoteFinder.jsm. r=mikedeboer

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

Yes! This works as expected.

Thanks Alex!

Comment 17

[atifrea]

Attachment: Bug 1004515 - Patch v6: add support for the find counter in RemoteFinder.jsm; fixed problem with findAgain. r=mikedeboer

Comment 18

[atifrea]

There was a problem with findAgain in the previous patch. I fixed it in the v6 of the patch.

Comment 19

[Mike de Boer [:mikedeboer]]

Comment on attachment 8448440 [details] [diff] [review]
Bug 1004515 - Patch v6: add support for the find counter in RemoteFinder.jsm; fixed problem with findAgain. r=mikedeboer

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

Ah, good find! However, you did upload a wrong patch... :/

Comment 20

[atifrea]

Attachment: Bug 1004515 - Patch v6: add support for the find counter in RemoteFinder.jsm; fixed problem with findAgain. r=mikedeboer

Comment 21

[Mike de Boer [:mikedeboer]]

Comment on attachment 8448780 [details] [diff] [review]
Bug 1004515 - Patch v6: add support for the find counter in RemoteFinder.jsm; fixed problem with findAgain. r=mikedeboer

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

Success! Thanks Alex!

::: toolkit/modules/RemoteFinder.jsm
@@ +32,4 @@
>    },
>  
>    receiveMessage: function (aMessage) {
> +    // Only Finder:Result messages have the searchString field. 

nit: trailing whitespace

Comment 22

[Blake Kaplan (:mrbkap) (inactive)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/417723d8c294

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/417723d8c294