68307 - Find dialog needs to be posed from JS not C++

Status: VERIFIED FIXED

(SeaMonkey :: UI Design, defect, P2)

Description

[Conrad Carlen (not reading bugmail)]

The Find component pops a XUL dialog box from C++ code when asked to find. This
is inside-out. I had to add a new API for text searching when embedding in order
to get away from this code<->UI coupling. The find dialog should be posed from
JS, get a scriptable find API, and call its methods. Actually, a find iface
should just be available from an nsIDomWindow. This would allow for better
sharing of code between embedding apps and seamonkey.

Comment 1

[Simon Fraser [no longer active]]

Keep law in the loop

Comment 2

[Bill Law]

The problem with opening the dialog from JS is that it is almost certain to be
the client code that does so, which thus limits the implementation choices for
this component.

Consider the consequences of applying this same rationale to the file picker,
for example.  If every client of that service were to code
    window.openDialog( "chrome://global/content/filePicker.xul" );
then the ability to implement nsIFilePicker::Show via a native platform dialog
would be lost.

If all find clients code
    window.openDialog( "chrome://global/content/fileDialog.xul" );
then we impose a similar limitation.  It also makes it more difficult to use an
alternative find dialog, even one implemented in xul/js.

That's why the find dialog was structured the way it is (like nsIFilePicker).
We use xpcom to seperate the implementation of the dialog from the clients.
That is standard Mozilla practice, I believe (to separate components and
communicate between them using xpcom interfaces), even if it is only honored in
the breach in the context of much of our front-end code.

One of the consequence of such use of xpcom, of course, is the ability to
implement interfaces in one's choice of language.  The "find component" happens
to be implemented in C++ (because it was the only option at the time).  Another
consequence of using xpcom is that one can modify the implementation of a
component without impact on, or knowledge of, the clients that use the
component.  I think nsIFindComponent has succeeded in that, too.   Despite
whatever churn has occurred in that dialog and its implementation, clients have
been oblivious to it.  That's a good thing, not a bad one.

Given all that, I'm not sure why the current situation is such a cause for concern.

Which isn't to say that nsIFindComponent doesn't have problems.  But I think
those should be expressed in terms of the interface and its methods, not the
choice of implementation language, or limitations of that implementation.

>I had to add a new API for text searching when embedding in order to get away
>from this code<->UI coupling.

I'm not sure why a new API was necessary.  This is relatively simple to do with
nsIFindComponent as it stands (just call FindNext rather than Find).  The only
difficulty might be that there would still be the (unreachable) code in there
that called nsIDomWindowInternal::OpenDialog (with a chrome URL).  But I suspect
that could have been remedied by tweaking the implementation that was there
rather than by adding a whole new interface.

>The find dialog should be posed from
>JS, get a scriptable find API, and call its methods.

Isn't that how it works already?  You just "pose" it by calling
nsIFindComponent::Find (instead of by using window.openDialog).

>Actually, a find iface
>should just be available from an nsIDomWindow.

I interpret this as saying that one should QI the nsIDOMWindow to some
"nsIFindThingy" interface?  I'm not sure I agree with that.  What happens if
nsIDOMWindow's implementation sucks?  If there is a separate "find" service,
then it can more easily be replaced.  That service could surely still be part of
the "embeddable" Gecko browser.

nsIDOMWindow should already have some sort of "find" method, since that's part
of DOM Level 0.  I'm not sure what would be needed beyond that.  It could
implement that by using the nsIFindComponent service, theoretically.

Comment 3

[kinmoz]

This should go to [e-mail], he owns the xpfe find component.

Comment 4

[Simon Fraser [no longer active]]

Mine.

Comment 5

[Simon Fraser [no longer active]]

OK, lots of changes coming here. I'm using this bug to move browser's find 
implementation to use the same impl. as embedding (nsIWebBrowserFind), so the 
changes are in two parts:
1. Make nsIWebBrowserFind available in the non-embedding case (by a GetInterface
   on an nsIDocShell).

2. Eliminate Find methods on nsBrowserInstance, and rewrite the navigator
   and find dialog JS to use nsIWebBrowserFind.

Comment 6

[Simon Fraser [no longer active]]

Attachment: Wean nsWebBrowser of its own implementation of nsIWebBrowserFind

Comment 7

[Simon Fraser [no longer active]]

Attachment: Make nsDocShell support GetInterface of nsIWebBrowserFind

Comment 8

[Simon Fraser [no longer active]]

Attachment: Fix find dialog js to use nsIWebBrowserFind, and start to obselete C++ find component

Comment 9

[Simon Fraser [no longer active]]

Attachment: remove find stuff from nsBrowserInstance, and change JS for find and findAgain

Comment 10

[Simon Fraser [no longer active]]

Attachment: New file mozilla/embedding/components/find/public/nsIWebBrowserFind.idl

Comment 11

[Simon Fraser [no longer active]]

Attachment: New file: mozilla/embedding/components/find/src/nsWebBrowserFind.h

Comment 12

[Simon Fraser [no longer active]]

Attachment: New file: mozilla/embedding/components/find/src/nsWebBrowserFind.cpp

Comment 13

[Simon Fraser [no longer active]]

Attachment: New file: mozilla/xpfe/components/find/src/nsFindModule.cpp

Comment 14

[Simon Fraser [no longer active]]

Attachment: New file: mozilla/xpfe/components/find/public/nsIFindService.idl

Comment 15

[Simon Fraser [no longer active]]

Attachment: New file: mozilla/xpfe/components/find/src/nsFindService.h

Comment 16

[Simon Fraser [no longer active]]

Attachment: New file: mozilla/xpfe/components/find/src/nsFindService.cpp

Comment 17

[Simon Fraser [no longer active]]

Here's the deal with the old find component:
* I just moved the module stuff into a new file, nsFindModule.cpp
* I added a new nsFindService, whose *only* task is to act as an application-wide 
storage for global find parameters. The Find dialog gets and sets the values in 
this service.
* The stuff in nsFindCompnent.cpp will eventually go away, but it has to stick 
around until we implement the Replace stuff for editor (which uses its own 
dialog). After that, the nsFindService will be the only thing left in this DLL, 
so maybe it should be folded into another library.

Here's what happened to nsWebBrowser:
* nsWebBrowser used to actually implemenet nsIWebBrowserFind, and the 
GetInterface would turn around and just do a QI. In order to share the 
nsWebBrowserFind implementation, I moved it into embedding/components, and made 
it so that you can CreateInstance one. Now, both nsWebBrowser and nsDocShell do 
this when someone does a GetInterface for nsIWebBrowserChrome.

Comment 18

[Simon Fraser [no longer active]]

Attachment: xulBindings change to expose nsIWebBrowserFind on <browser>

Comment 19

[Simon Fraser [no longer active]]

OK, apart from Makefile magic, that's pretty much it.

Reviews?

Comment 20

[Simon Fraser [no longer active]]

Attachment: same change in browserBindings.xml

Comment 21

[Conrad Carlen (not reading bugmail)]

From nsIWebBrowserFind.idl - when does init() have to be called? Only before the
first call to findNext or whenever you want it to search some specific frame.
That method is stuck in the file below the comment for findNext and could use
its own comment. Otherwise, looks good.

Comment 22

[Conrad Carlen (not reading bugmail)]

Since bug 63421 has been rolled into this one, WRT searching multiple frames...
I think searching through multiple frames is a UI preference and I'm not sure
what the convention is. In any split-pane editor I've used, searching happens
only in the active pane. Not exactly the same situation but similar. This is the
way the nsIWebBrowserFind impl works now. It uses the focused frame. If you want
to search in the next frame, just click in it. I think the real problem is that
we don't have visual indication which subframe is focused. If we had that, it
would make a lot more sense. 

Comment 23

[Alec Flett]

this is awesome!
As for searching multiple frames, frankly I think at the very minimum it should
be the default for the browser (as opposed to embedding), as some web pages
don't even make it clear that there ARE multiple frames (i.e. via tricks with
borderless frames & cool backgrounds, etc)
I'll sr= later today if I can.

Comment 24

[Simon Fraser [no longer active]]

I have not implemented the seaching of multiple frames yet, but the code will 
live down in nsWebBrowserFind. Note that I added an nsIDOMWindow param to Init(), 
and an out nsIDOMWindow to FindNext.

Thinking further, I think maybe I need a setter for the window to search, and the 
searchSubframes param, or maybe FindNext should look like this:

  boolean FindNext(in nsIDOMWindow inSearchFrame, in boolean inSearchSubframes, 
out nsIDOMWindow outFoundFrame);

Comment 25

[Simon Fraser [no longer active]]

I'm going to change nsIWebBrowserFind a little to allow control over searching 
subframes, so don't review just yet.

Comment 26

[Conrad Carlen (not reading bugmail)]

Couldn't nsIWebBrowserFind's impl keep track of the DOMWindows instead of having
both in and out DOMWindow params? How about this:

const unsigned long SEARCH_FROM_FIRST_FRAME;
const unsigned long SEARCH_FROM_CURRENT_FRAME;

and then FindNext is just
 boolean FindNext(in boolean inSearchSubframes, in unsigned long searchFromFrame);

Comment 27

[Simon Fraser [no longer active]]

Attachment: New nsIWebBrowserFind.idl

Comment 28

[Simon Fraser [no longer active]]

I attached a new version of nsIWebBrowserFind.idl for your delectation.

This preserves the original nsIWebBrowserFind as it was, and that interface will 
behave as it used to (don't search subframes, search the focussed or content area 
frame).

To control subframe searching, you will be able to QI nsIWebBrowserFind to 
nsIWebBrowserFindInFrames, and set the various frame searching attributes on 
that. Its attributes allow you to search in subframes, specify a frame to contain 
the search withing, and get/set the current search frame.

How does that look to everyone?

Comment 29

[Conrad Carlen (not reading bugmail)]

Beautiful! Thanks for the additional comments too.

Comment 30

[Simon Fraser [no longer active]]

0.9.1

Comment 31

[Simon Fraser [no longer active]]

See bug 63241 for newer versions of nsWebBrowserFind.cpp/h, which support 
searching in multiple frames.

Comment 32

[Simon Fraser [no longer active]]

cc adam for docShell reviews.

Comment 33

[Adam Lock]

Simon, is it possible to have one find service per docshelltree or does it have 
to be one per docshell?

If the latter then r=[e-mail]

Comment 34

[Simon Fraser [no longer active]]

That's a good question. I think, to provide embedders and mozilla with maximum 
flexibility, we need a find instance per content area (and a XUL window may have 
> 1 content area under the same chrome docShell hierarchy). Since we don't (I believe) have class that is instantiated just for 'content root' docShells, then the find instance has to hang of of nsDocShell.

Comment 35

[Conrad Carlen (not reading bugmail)]

There is the docshell tree owner for which there is one per docshell tree. It's
implemented in nsDocShellTreeOwner.cpp for embedding and nsContentTreeOwner.cpp
for seamonkey. Now, if only these two implementations could be unified (maybe
part of bug 76585?).

Comment 36

[Alec Flett]

bill (understandably) wants a reply to the issues he brought up early in this bug.

- as for why using an API that is proprietary to Find instead of using
window.openDialog, ask yourself "why does Find need to be any different than any
other dialog posed from seamonkey" - the answer is, "it shouldn't be any different"
  Why not? You described the file picker as a point of comparison. That is an
irrelevant example because the file picker is one of the rare cases where
mozilla allows the user to interact with the native system underneath it. It is
an exception, not a rule.
  The find dialog interacts only with mozilla, therefore it should look and
behave like other mozilla dialogs. There is no value in making it pluggable.
Just like the preferences window, we're not expecting people to replace the
prefwindow with native code, in seamonkey. 

- as for language choice and abstraction: There are two issues here, and I think
nsIFindComponent blurs them together:
  - The presentation of the find functionality in seamonkey. This is what needs
to be immediately accessable from JavaScript - and it should be done in the same
method as every other dialog in the application: via window.openDialog. This
ensures proper parenting, and with a single control path for opening windows,
mainetance for issues like modality, focus, etc, becomes standard across all
windows. 
  - The backend functionality: This is a completely seperate issue, which is
what embedding is basically trying to break out - we need presentation-free APIs
to tell a content area to "find" other content, maintain state about the
"current" content, and so forth.

- as for QI'ing nsIDOMWindow to find, I don't think that's what simon meant
- as for implementing DOM stuff with nsIFindComponent, again we need a
presentation-free method of doing this so that we can share it between embedding
and seamonkey. the embedding team already decided on interfaces - it's a shame
bill wasn't involved, but for what it's worth I think they had the right idea
(and Simon's polishing seems to be doing the right thing as well)

Comment 37

[Simon Fraser [no longer active]]

Checked in.

Comment 38

[sairuh (rarely reading bugmail)]

rubberstamp