Josh Matthews [:jdm] Assignee
	Description • 15 years ago

It currently doesn't work for remote tabs, and that is a shame.

	Josh Matthews [:jdm] Assignee
	Comment 1 • 15 years ago

Seeing as the description makes it sound like this patch doesn't actually do anything, I'll clarify: the findbar partially works in the browser, in that you can type a word and turn on highlighting and all of the correct elements appear to show up.  The actual individual element highlighting is currently busted because I'm not sure how to implement the commented out lines in the protocol.

	Benjamin Smedberg
	Comment 2 • 15 years ago

Of the methods you have commented out:

* getCaseSensitive/getSearchString sound easy but unnecessary: both sides can track the current search parameters
* setDocShell isn't needed, right?
* getFoundLink/getFoundEditable/getFoundWindow can't return a reference to the found object. How are they used? Probably whatever currently uses them will have to switch over to be in the content process.

	Josh Matthews [:jdm] Assignee
	Comment 3 • 15 years ago

Here's the second shot at the new async find behaviour.  The best way to test it is to load up the test shell and enter a word in the new box I added.  The alert message that pops up contains the numeric result of the search (ie. 0 == found, 1 == not found, 2 == wrapped).

	Josh Matthews [:jdm] Assignee
	Comment 4 • 15 years ago

Things accessing nsITypeAheadFind.(foundLink|foundEditable|currentWindow) in findbar.xml:

- _handleEnter: dispatches a keypress to the returned link
- _updateFoundLink: keeps a copy of each of the three, which are then used by lots of other functions

What would be the right way to move this behaviour out of the chrome and into content?

	Josh Matthews [:jdm] Assignee
	Comment 5 • 15 years ago

I've removed a bunch of debugging cruft from the previous version, and integrated bsmedberg's better test shell patch.  I've also reworked some more of the code in the findbar that relied on the (now unavailable) return code of the find to use a callback.  Apart from the stuff detailed in the last comment, I think this is probably ready to be given a serious look.

	Josh Matthews [:jdm] Assignee
	Comment 6 • 15 years ago

This is the same patch as the previous one, except that the findbar javascript changes should actually work this time.  The find result callbacks require a bit more outside state than just the result, so I'm wrapping the actual work in a closure.

	Benjamin Smedberg
	Comment 7 • 15 years ago

Comment on attachment 406400 [details] [diff] [review]
Async typeaheadfind with findbar fixes

>diff -r b7cdefea36d3 content/base/public/nsIFrameLoader.idl

>+%{C++
>+namespace mozilla { namespace dom { class
>+TabParent; } }
>+%}

Formatting nit, this should use standard C++ formatting, so

namespace mozilla {
namespace dom {
class TabParent;
}
}

> [scriptable, uuid(fe5f43b1-6b9d-4639-be08-03f0de1a22f9)]

Whenever an interface changes you need to generate a new UUID.

>\ No newline at end of file

Please fix this while you're here?

>diff -r b7cdefea36d3 content/base/src/nsFrameLoader.cpp

>+NS_IMETHODIMP
>+nsFrameLoader::GetRemoteParent(TabParent** aTabParent)
>+{
>+  *aTabParent = mChildProcess;
>+  
>+  return NS_OK;
>+}

This will need #ifdef MOZ_IPC before checkin.

>diff -r b7cdefea36d3 dom/ipc/Makefile.in

>+LOCAL_INCLUDES += -I$(topsrcdir)/toolkit/components/typeaheadfind/src \
>+		  -I$(topsrcdir)/docshell/base

This hunk should be after you include rules.mk, but I'm saddened we have to do this. Can we export the necessary headers instead, or are the build tiers still getting in the way?

Formatting nit, please use:

LOCAL_INCLUDES += \
  foo1 \
  foo2 \
  $(NULL)

>diff -r b7cdefea36d3 dom/ipc/test.xul

> <window xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"
>-        width="800" height="800" orient="vertical">
>+        width="800" height="800" orient="vertical" onload="onLoad()">

I don't think we want the onLoad bit, at least not as a side effect of this patch, since only some of us see the tiny-initial-window bug.

>diff -r b7cdefea36d3 toolkit/components/typeaheadfind/public/nsIFindResult.idl

>+/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */

The standard mode for new files is tab-width: 8 so that we can see and correct hard tabs that might sneak in accidentally. And the actual file below has 4-space indent, please make it use 2.

>diff -r b7cdefea36d3 toolkit/components/typeaheadfind/public/nsITypeAheadFind.idl

>+%{C++
>+namespace mozilla { namespace dom {
>+class TypeAheadFindChild;
>+class PFindResultChild; } }
>+%}

Style nit as above.

>   /* Necessary initialization that can't happen in the constructor, either
>-   * because function calls here may fail, or because the docShell is
>+   * because function calls here may fail, or because the frameLoader is
>    * required. */
>-  void init(in nsIDocShell aDocShell);
>-
>+  void init(in nsIFrameLoader aFrameLoader);
>+  [noscript] void initChild(in nsIDocShell aDocShell, in TypeAheadFindChild aFindProtocol);
>+  [noscript] void setFindResultProtocol(in PFindResultChild aFindResult);

I think we may also want to keep the original init method which uses a docshell, and add a new initFromFrameLoader method, so that embedding consumers can use the in-process version when there is no frameloader.

>   /* Find aSearchString in page.  If aLinksOnly is true, only search the page's
>    * hyperlinks for the string. */
>-  unsigned short find(in AString aSearchString, in boolean aLinksOnly);
>+  void find(in AString aSearchString, in boolean aLinksOnly, in nsIFindResult aObserver);
> 
>   /* Find another match in the page. */
>-  unsigned short findAgain(in boolean findBackwards, in boolean aLinksOnly);
>-
>+  void findAgain(in boolean findBackwards, in boolean aLinksOnly, in nsIFindResult aObserver);

It may be hard to change existing consumers of these methods all at once. Might it be better to add new methods asyncFind and asyncFindAgain, and move the existing consumers over more gradually? If you're going to make the API change all at once we should land that all in mozilla-central, and then land the IPC-specific bits as a second patch.

In addition, there isn't enough documentation about when the asynchronous callback may be fired. In the case of an in-process tab, will it will be fired within the current stack frame?

>diff -r b7cdefea36d3 toolkit/components/typeaheadfind/src/Makefile.in

>+LOCAL_INCLUDES += -I$(topsrcdir)/docshell/base

More sadness and makefile rearrangement required.

>diff -r b7cdefea36d3 toolkit/components/typeaheadfind/src/PTypeAheadFind.ipdl

>+/* -*- Mode: C++; c-basic-offset: 4; indent-tabs-mode: nil; tab-width: 8 -*- */

Please use c-basic-offset 2 for new code.


>+child:
>+    find(nsString text, PRBool searchLinks);
>+    findAgain(PRBool findBackwards, PRBool linksOnly);
>+    setCaseSensitive(PRBool isCaseSensitive);
>+    setSelectionModeAndRepaint(PRInt16 toggle);
>+    collapseSelection();
>+
>+    PFindResult();
>+    
>+parent:
>+    ~PFindResult(PRUint16 searchResult);
>+};

Please use `bool` instead of `PRBool` in protocols. 

I don't see any particularly good reason for setCaseSensitive to be a message: just store the case-sensitivity in a local member and send it when necessary.

I'm surprised that there's a separate find message and PFindResult constructor... why can't we just have a single method

PFindResult(nsString text, PRBool searchLinks, bool searchAgain, bool caseSensitive);

That's as far as I've gotten, I'll pick up again on Monday, or let me know if you want to post a new patch and I'll wait.

	Josh Matthews [:jdm] Assignee
	Comment 9 • 15 years ago

Comment on attachment 408851 [details] [diff] [review]
Previous patch with cleanups from partial review

I've addressed all of the comments in the review, as well as ensured that it functions without IPC enabled.

	Benjamin Smedberg
	Comment 10 • 15 years ago

Comment on attachment 408851 [details] [diff] [review]
Previous patch with cleanups from partial review

>diff --git a/dom/ipc/test.xul b/dom/ipc/test.xul

>+      _find = Cc["@mozilla.org/typeaheadfind;1"].createInstance(Ci.nsITypeAheadFind);
>+      try
>+      {
>+        _find.initFromFrameLoader(frameLoader);
>+      }
>+      catch(e)
>+      {
>+        _find.init(frameLoader.docShell);
>+      }

The try/catch doesn't seem like it should be necessary. Why can't .initFromFrameLoader work for both local and remote frames? Just get the docshell off the frameloader if it's a local frame.

>diff --git a/toolkit/components/typeaheadfind/public/nsITypeAheadFind.idl b/toolkit/components/typeaheadfind/public/nsITypeAheadFind.idl

This needs a UUID change.

>diff --git a/toolkit/components/typeaheadfind/src/PFindResult.ipdl b/toolkit/components/typeaheadfind/src/PFindResult.ipdl

>+/* -*- Mode: C++; c-basic-offset: 4; indent-tabs-mode: nil; tab-width: 8 -*- */

new files should be c-basic-offset: 2

This file needs a license header, see http://www.mozilla.org/MPL/boilerplate-1.1/mpl-tri-license-c for a template.

>diff --git a/toolkit/components/typeaheadfind/src/PTypeAheadFind.ipdl b/toolkit/components/typeaheadfind/src/PTypeAheadFind.ipdl

need a license header here as well

>diff --git a/toolkit/components/typeaheadfind/src/TypeAheadFindChild.cpp b/toolkit/components/typeaheadfind/src/TypeAheadFindChild.cpp

license header

>+using namespace mozilla::dom;

Please just put these in the namespace, e.g.

namespace mozilla {
namespace dom {

>+TypeAheadFindChild::TypeAheadFindChild(nsIDocShell *aDocShell)
>+{
>+    mTypeAheadFind = do_CreateInstance(NS_TYPEAHEADFIND_CONTRACTID);
>+    mTypeAheadFind->Init(aDocShell);
>+}

Both of these are potentially fallible operations. You should at least null-check mTypeAheadFind before calling ->Init, and you should probably also rv-check the call to ->Init and null out mTypeAheadFind if it fails.

>+bool
>+TypeAheadFindChild::RecvPFindResultConstructor(PFindResultChild* aActor,
>+                                               const nsString& text,
>+                                               const bool& searchAgain,
>+                                               const bool& backwards,
>+                                               const bool& searchLinks,
>+                                               const bool& caseSensitive)
>+{

null-check mTypeAheadFind here and send a search-failed result if its null.

>diff --git a/toolkit/components/typeaheadfind/src/TypeAheadFindChild.h b/toolkit/components/typeaheadfind/src/TypeAheadFindChild.h

License header.

>diff --git a/toolkit/components/typeaheadfind/src/TypeAheadFindParent.cpp b/toolkit/components/typeaheadfind/src/TypeAheadFindParent.cpp

License header.

>+PFindResultParent*
>+TypeAheadFindParent::AllocPFindResult(const nsString& text,
>+                                      const bool& searchAgain,
>+                                      const bool& backwards,
>+                                      const bool& searchLinks,
>+                                      const bool& caseSensitive)
>+{

Comments below will indicate that this method should be NS_NOTREACHED.

>+bool
>+TypeAheadFindParent::DeallocPFindResult(PFindResultParent* aProtocol,
>+                                        const PRUint16& result)
>+{
>+    delete aProtocol;
>+    mObserver->Found(result);

The way you've set up mObserver will lead to unexpected and unfortunate race conditions. What happens if the user searches twice in a row? You'll overwrite mObserver with the new observer before the old result comes back. You should instead attach mObserver to a FindResultParent object:

class FindResultParent : public PFindResultParent
{
public:
  nsCOMPtr<nsIFindResult> mObserver;
};

>+bool
>+TypeAheadFindParent::Find(const nsString& text,
>+                          const bool& searchLinks,
>+                          const bool& caseSensitive,
>+                          nsIFindResult* aObserver)
>+{
>+    mObserver = aObserver;
>+    SendPFindResultConstructor(text, false, false, searchLinks, caseSensitive);

Then here you can use the alternate form of the Send... method which accepts a specific object:

  FindResultParent* result = new FindResultParent(aObserver);
  SendPFindResultConstructor(result, text, false, false, searchLinks, caseSensitive);

In addition, you need to check the result of Send functions on the parent side: if they return false, the child may have crashed or had some other illegal situation.

>diff --git a/toolkit/components/typeaheadfind/src/TypeAheadFindParent.h b/toolkit/components/typeaheadfind/src/TypeAheadFindParent.h

>+/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
>+/* vim: sw=4 ts=4 et : */

These don't match: you want sw=2 ts=8

And you want a license header ;-)

>+class nsIFindResult;

It is very surprising/unexpected that you can forward-declare this class and then use it with nsCOMPtr. I think you have to actually #include "nsIFindResult.h" in order for this to compile on all platforms.

>diff --git a/toolkit/components/typeaheadfind/src/nsTypeAheadFind.cpp b/toolkit/components/typeaheadfind/src/nsTypeAheadFind.cpp

>+#ifdef MOZ_IPC
> nsresult
>-nsTypeAheadFind::Init(nsIDocShell* aDocShell)
>+nsTypeAheadFind::InitFromFrameLoader(nsIFrameLoader* aFrameLoader)
>+{
>+  mozilla::dom::TabParent *parent;
>+  aFrameLoader->GetRemoteParent(&parent);
>+  if(parent)
>+  {
>+    mDestProtocol = parent->CreateFindProtocol();
>+    return NS_OK;
>+  }
>+  else
>+  {
>+    return NS_ERROR_FAILURE;
>+  }
>+}
>+#endif

Where is the implementation of InitFromFrameLoader if MOZ_IPC is not defined? I think you'll end up with linker errors. In addition, I don't see why this method shouldn't be implemented in !MOZ_IPC builds. Always implement the method: the GetRemoteParent call should be in MOZ_IPC ifdefs. If that fails, fall back to getting the docshell from the frameloader and just call Init() with it.

>@@ -151,7 +178,7 @@
>   // ----------- Set search options ---------------
>   mFind->SetCaseSensitive(PR_FALSE);
>   mFind->SetWordBreaker(nsnull);
>-
>+  

extraneous change? Please don't introduce extra whitespace.

>@@ -182,6 +209,12 @@
> NS_IMETHODIMP
> nsTypeAheadFind::SetCaseSensitive(PRBool isCaseSensitive)
> {
>+  if(mDestProtocol != nsnull)
>+  {
>+    mCaseSensitive = isCaseSensitive;
>+    return NS_OK;
>+  }

Here and elsewhere, make this check `if (mDestProtocol)`

It's really unfortunate that every entry point has to have an if (mDestProtocol) check. Please file a followup bug to use a different C++ implementation for the local and remote case (requires additional API changes to nsITypeAheadFind to make the init() method a factory).


>@@ -305,7 +359,6 @@
>                            PRBool aIsFirstVisiblePreferred, PRBool aFindPrev,
>                            PRUint16* aResult)
> {
>-  *aResult = FIND_NOTFOUND;
>   mFoundLink = nsnull;
>   mFoundEditable = nsnull;
>   mCurrentWindow = nsnull;

Why did you remove this line? It seems like a behavior change (if one of the error conditions kicks in, the outparam won't be set) which is probably not harmful, but also not necessary.

>@@ -647,6 +700,13 @@
> NS_IMETHODIMP
> nsTypeAheadFind::GetFoundLink(nsIDOMElement** aFoundLink)
> {
>+  if(mDestProtocol != nsnull)
>+  {
>+    //TODOjdm: proxy call
>+    //return mDestProtocol->GetFoundLink(aFoundLink);
>+    return NS_OK;
>+  }

This is incorrect, you should return NS_ERROR_NOT_IMPLEMENTED or some other error value indicating that the caller isn't getting the data they expect. If you were to return NS_OK, you *must* set the outparam, even if it's just *aFoundLink = nsnull.

>+NS_IMETHODIMP
>+nsTypeAheadFind::AsyncFindAgain(PRBool aFindBackwards, PRBool aLinksOnly,
>+                                nsIFindResult* aObserver)
>+{
>+  if(mDestProtocol != nsnull)
>+  {
>+    mDestProtocol->FindAgain(aFindBackwards, aLinksOnly, mCaseSensitive, aObserver);
>+  }
>+  else
>+  {
>+    PRUint16 result;
>+    nsresult rv = FindAgain(aFindBackwards, aLinksOnly, &result);
>+    NS_ENSURE_SUCCESS(rv, rv);
>+    if(aObserver)
>+      aObserver->Found(result);
>+  }

Standard mozilla bracing/indentation for multi-line blocks is

if (cond) {
  ...
}
else {
  ...
}

There are more extraneous whitespace changes: please remove them before attaching another patch.

>diff --git a/toolkit/components/typeaheadfind/src/nsTypeAheadFind.h b/toolkit/components/typeaheadfind/src/nsTypeAheadFind.h

>+namespace mozilla {
>+namespace dom {
>+class TypeAheadFindParent;
>+}
>+}

This, and the mDestProtocol member below, should be #ifdef MOZ_IPC

Who is responsible for calling SendPTypeAheadFindDestructor on mDestProtocol? We don't want to leak it.

	Josh Matthews [:jdm] Assignee
	Comment 11 • 15 years ago

(In reply to comment #10)
> (From update of attachment 408851 [details] [diff] [review])
> >diff --git a/dom/ipc/test.xul b/dom/ipc/test.xul
> 
> >+      _find = Cc["@mozilla.org/typeaheadfind;1"].createInstance(Ci.nsITypeAheadFind);
> >+      try
> >+      {
> >+        _find.initFromFrameLoader(frameLoader);
> >+      }
> >+      catch(e)
> >+      {
> >+        _find.init(frameLoader.docShell);
> >+      }
> 
> The try/catch doesn't seem like it should be necessary. Why can't
> .initFromFrameLoader work for both local and remote frames? Just get the
> docshell off the frameloader if it's a local frame.

Done.
 
> >diff --git a/toolkit/components/typeaheadfind/public/nsITypeAheadFind.idl b/toolkit/components/typeaheadfind/public/nsITypeAheadFind.idl
> 
> This needs a UUID change.

Done.

> >diff --git a/toolkit/components/typeaheadfind/src/PFindResult.ipdl b/toolkit/components/typeaheadfind/src/PFindResult.ipdl
> 
> >+/* -*- Mode: C++; c-basic-offset: 4; indent-tabs-mode: nil; tab-width: 8 -*- */
> 
> new files should be c-basic-offset: 2
> 
> This file needs a license header, see
> http://www.mozilla.org/MPL/boilerplate-1.1/mpl-tri-license-c for a template.

Done and done.  I copied the same license header to every file that needed it, so if there's a problem in one then there's a problem with all of them.

> >diff --git a/toolkit/components/typeaheadfind/src/PTypeAheadFind.ipdl b/toolkit/components/typeaheadfind/src/PTypeAheadFind.ipdl
> 
> need a license header here as well

Done.

> >diff --git a/toolkit/components/typeaheadfind/src/TypeAheadFindChild.cpp b/toolkit/components/typeaheadfind/src/TypeAheadFindChild.cpp
> 
> license header
> 
> >+using namespace mozilla::dom;
> 
> Please just put these in the namespace, e.g.
> 
> namespace mozilla {
> namespace dom {

Done and done.

> >+TypeAheadFindChild::TypeAheadFindChild(nsIDocShell *aDocShell)
> >+{
> >+    mTypeAheadFind = do_CreateInstance(NS_TYPEAHEADFIND_CONTRACTID);
> >+    mTypeAheadFind->Init(aDocShell);
> >+}
> 
> Both of these are potentially fallible operations. You should at least
> null-check mTypeAheadFind before calling ->Init, and you should probably also
> rv-check the call to ->Init and null out mTypeAheadFind if it fails.

Done.

> >+bool
> >+TypeAheadFindChild::RecvPFindResultConstructor(PFindResultChild* aActor,
> >+                                               const nsString& text,
> >+                                               const bool& searchAgain,
> >+                                               const bool& backwards,
> >+                                               const bool& searchLinks,
> >+                                               const bool& caseSensitive)
> >+{
> 
> null-check mTypeAheadFind here and send a search-failed result if its null.

Done. Also null-checked the other messages.

> >diff --git a/toolkit/components/typeaheadfind/src/TypeAheadFindChild.h b/toolkit/components/typeaheadfind/src/TypeAheadFindChild.h
> 
> License header.

Done.

> >diff --git a/toolkit/components/typeaheadfind/src/TypeAheadFindParent.cpp b/toolkit/components/typeaheadfind/src/TypeAheadFindParent.cpp
> 
> License header.

Done.

> >+PFindResultParent*
> >+TypeAheadFindParent::AllocPFindResult(const nsString& text,
> >+                                      const bool& searchAgain,
> >+                                      const bool& backwards,
> >+                                      const bool& searchLinks,
> >+                                      const bool& caseSensitive)
> >+{
> 
> Comments below will indicate that this method should be NS_NOTREACHED.

Done.

> >+bool
> >+TypeAheadFindParent::DeallocPFindResult(PFindResultParent* aProtocol,
> >+                                        const PRUint16& result)
> >+{
> >+    delete aProtocol;
> >+    mObserver->Found(result);
> 
> The way you've set up mObserver will lead to unexpected and unfortunate race
> conditions. What happens if the user searches twice in a row? You'll overwrite
> mObserver with the new observer before the old result comes back. You should
> instead attach mObserver to a FindResultParent object:
> 
> class FindResultParent : public PFindResultParent
> {
> public:
>   nsCOMPtr<nsIFindResult> mObserver;
> };

Done.

> >+bool
> >+TypeAheadFindParent::Find(const nsString& text,
> >+                          const bool& searchLinks,
> >+                          const bool& caseSensitive,
> >+                          nsIFindResult* aObserver)
> >+{
> >+    mObserver = aObserver;
> >+    SendPFindResultConstructor(text, false, false, searchLinks, caseSensitive);
> 
> Then here you can use the alternate form of the Send... method which accepts a
> specific object:
> 
>   FindResultParent* result = new FindResultParent(aObserver);
>   SendPFindResultConstructor(result, text, false, false, searchLinks,
> caseSensitive);
> 
> In addition, you need to check the result of Send functions on the parent side:
> if they return false, the child may have crashed or had some other illegal
> situation.

Done.
 
> >diff --git a/toolkit/components/typeaheadfind/src/TypeAheadFindParent.h b/toolkit/components/typeaheadfind/src/TypeAheadFindParent.h
> 
> >+/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
> >+/* vim: sw=4 ts=4 et : */
> 
> These don't match: you want sw=2 ts=8
> 
> And you want a license header ;-)

Done.

> >+class nsIFindResult;
> 
> It is very surprising/unexpected that you can forward-declare this class and
> then use it with nsCOMPtr. I think you have to actually #include
> "nsIFindResult.h" in order for this to compile on all platforms.

Moot as this member no longer exists.

> >diff --git a/toolkit/components/typeaheadfind/src/nsTypeAheadFind.cpp b/toolkit/components/typeaheadfind/src/nsTypeAheadFind.cpp
> 
> >+#ifdef MOZ_IPC
> > nsresult
> >-nsTypeAheadFind::Init(nsIDocShell* aDocShell)
> >+nsTypeAheadFind::InitFromFrameLoader(nsIFrameLoader* aFrameLoader)
> >+{
> >+  mozilla::dom::TabParent *parent;
> >+  aFrameLoader->GetRemoteParent(&parent);
> >+  if(parent)
> >+  {
> >+    mDestProtocol = parent->CreateFindProtocol();
> >+    return NS_OK;
> >+  }
> >+  else
> >+  {
> >+    return NS_ERROR_FAILURE;
> >+  }
> >+}
> >+#endif
> 
> Where is the implementation of InitFromFrameLoader if MOZ_IPC is not defined? I
> think you'll end up with linker errors. In addition, I don't see why this
> method shouldn't be implemented in !MOZ_IPC builds. Always implement the
> method: the GetRemoteParent call should be in MOZ_IPC ifdefs. If that fails,
> fall back to getting the docshell from the frameloader and just call Init()
> with it.

Done.

> >@@ -151,7 +178,7 @@
> >   // ----------- Set search options ---------------
> >   mFind->SetCaseSensitive(PR_FALSE);
> >   mFind->SetWordBreaker(nsnull);
> >-
> >+  
> 
> extraneous change? Please don't introduce extra whitespace.

Fixed.

> >@@ -182,6 +209,12 @@
> > NS_IMETHODIMP
> > nsTypeAheadFind::SetCaseSensitive(PRBool isCaseSensitive)
> > {
> >+  if(mDestProtocol != nsnull)
> >+  {
> >+    mCaseSensitive = isCaseSensitive;
> >+    return NS_OK;
> >+  }
> 
> Here and elsewhere, make this check `if (mDestProtocol)`

Done.

> It's really unfortunate that every entry point has to have an if
> (mDestProtocol) check. Please file a followup bug to use a different C++
> implementation for the local and remote case (requires additional API changes
> to nsITypeAheadFind to make the init() method a factory).

Filed bug 526283.

> >@@ -305,7 +359,6 @@
> >                            PRBool aIsFirstVisiblePreferred, PRBool aFindPrev,
> >                            PRUint16* aResult)
> > {
> >-  *aResult = FIND_NOTFOUND;
> >   mFoundLink = nsnull;
> >   mFoundEditable = nsnull;
> >   mCurrentWindow = nsnull;
> 
> Why did you remove this line? It seems like a behavior change (if one of the
> error conditions kicks in, the outparam won't be set) which is probably not
> harmful, but also not necessary.

Whoops, good catch.  Fixed.

> >@@ -647,6 +700,13 @@
> > NS_IMETHODIMP
> > nsTypeAheadFind::GetFoundLink(nsIDOMElement** aFoundLink)
> > {
> >+  if(mDestProtocol != nsnull)
> >+  {
> >+    //TODOjdm: proxy call
> >+    //return mDestProtocol->GetFoundLink(aFoundLink);
> >+    return NS_OK;
> >+  }
> 
> This is incorrect, you should return NS_ERROR_NOT_IMPLEMENTED or some other
> error value indicating that the caller isn't getting the data they expect. If
> you were to return NS_OK, you *must* set the outparam, even if it's just
> *aFoundLink = nsnull.

Done.

> >+NS_IMETHODIMP
> >+nsTypeAheadFind::AsyncFindAgain(PRBool aFindBackwards, PRBool aLinksOnly,
> >+                                nsIFindResult* aObserver)
> >+{
> >+  if(mDestProtocol != nsnull)
> >+  {
> >+    mDestProtocol->FindAgain(aFindBackwards, aLinksOnly, mCaseSensitive, aObserver);
> >+  }
> >+  else
> >+  {
> >+    PRUint16 result;
> >+    nsresult rv = FindAgain(aFindBackwards, aLinksOnly, &result);
> >+    NS_ENSURE_SUCCESS(rv, rv);
> >+    if(aObserver)
> >+      aObserver->Found(result);
> >+  }
> 
> Standard mozilla bracing/indentation for multi-line blocks is
> 
> if (cond) {
>   ...
> }
> else {
>   ...
> }
> 
> There are more extraneous whitespace changes: please remove them before
> attaching another patch.

Done and done.

> >diff --git a/toolkit/components/typeaheadfind/src/nsTypeAheadFind.h b/toolkit/components/typeaheadfind/src/nsTypeAheadFind.h
> 
> >+namespace mozilla {
> >+namespace dom {
> >+class TypeAheadFindParent;
> >+}
> >+}
> 
> This, and the mDestProtocol member below, should be #ifdef MOZ_IPC

Done.

> Who is responsible for calling SendPTypeAheadFindDestructor on mDestProtocol?
> We don't want to leak it.

That would be the responsibility of the TypeAheadFindChild that initiates the search.

	Benjamin Smedberg
	Comment 14 • 15 years ago

Comment on attachment 409982 [details] [diff] [review]
Same patch with review comments addressed + missed nit

>diff --git a/dom/ipc/test.xul b/dom/ipc/test.xul

>+    function find(forwards) {
>+      if(_find == null)

Whoops, I missed this on earlier reviews: style guide says to use a space after control keywords if/for/do/while. I think you can just fix this by replaceing "if(" with "if (" everywhere in this patch.

I believe you forgot to `hg add` FindResultParent.{h,cpp}. I would like to review those.

	Josh Matthews [:jdm] Assignee
	Comment 15 • 15 years ago

Third time's the charm?  Missing files added and if() nit addressed.

	Benjamin Smedberg
	Comment 16 • 15 years ago

This will eventually need some chrome mochitests, but I don't think they can be implemented reliably until bug 514705 is done.

	Josh Matthews [:jdm] Assignee
	Comment 17 • 15 years ago

Unbitrotted from recent __delete__ changes.

	Josh Matthews [:jdm] Assignee
	Comment 19 • 13 years ago

Rebased.

	Josh Matthews [:jdm] Assignee
	Comment 20 • 13 years ago

Rebased.

	Tom S [:evilpie]
	Comment 21 • 11 years ago

We don't need this anymore.