46869 - Embedding apps need Find functionality

Status: VERIFIED FIXED

(Core Graveyard :: Embedding: APIs, defect, P1)

Description

[Judson Valeski]

This is not the level we want to expose. 
-Conrad to 
convert 
http://lxr.mozilla.org/seamonkey/source/embedding/browser/powerplant/source/CFin
dComponent.h into an xpidl interface and move both the interface and the 
implementation into  the core. 
-FindAll to be removed from the interface. 
-nsString& and PRBool& to be converted to XPCOM equivalents.

Comment 1

[Conrad Carlen (not reading bugmail)]

By "core", do we mean in terms of embedding or SeaMonkey? If it's embedding -
since we have embedding/base, I think a good place would be embedding/extras/
find. There is reason to put it in the core of SeaMonkey though. The find 
algorithm mine uses was taken from xpfe/components/find/ It adds entire word 
matching though. SeaMonkey's could be layered on top of mine. It would handle the 
XUL UI and call on my component to do the search.  

Comment 2

[Judson Valeski]

changing to new email

Comment 3

[Conrad Carlen (not reading bugmail)]

re-accepted at new email

Comment 4

[lchiang]

per email with Jud, changing nsbeta3+ to nsbeta3- on all "embed" keyword bugs
since embedding changes will not be made in the mn6 branch. If you feel this bug
fix needs to go into mn6 branch, please list the reasons/user impact/risk and
nominate for rtm. Thanks.

Comment 5

[Paul Wyskoczka]

Updating QA Contact

Comment 6

[Conrad Carlen (not reading bugmail)]

Attachment: finddiffs.txt

Comment 7

[Conrad Carlen (not reading bugmail)]

Attachment: nsIWebBrowserFind.idl

Comment 8

[Conrad Carlen (not reading bugmail)]

Attachment: nsWebBrowserFind.cpp

Comment 9

[Conrad Carlen (not reading bugmail)]

Attachment: nsWebBrowserFind.h

Comment 10

[Conrad Carlen (not reading bugmail)]

Kin, can you review? The nsWebBrowserFind code was mainly copied straight from
your code. I just put the needed functionality in the webbrowser lib since
that's what embedding clients use. 

Comment 11

[kinmoz]

Conrad, looks good. r=kin

Comment 12

[Conrad Carlen (not reading bugmail)]

Vidur, can you sr? Main question is my impl of nsWebBrowser::GetFocusedWindow().
This needs to be used so that finding in framesets works on the "focused" frame.
It does work but I'm not sure if I'm getting the focused DOM window in the best way.

Comment 13

[Conrad Carlen (not reading bugmail)]

Chris (Saari) - Can you take a look at the nsWebBrowser::GetFocusedWindow()
 since you're dealing w/ focus issues?

Comment 14

[Simon Fraser [no longer active]]

So can we change find in mozilla to use the same code? I hate to see code 
duplicate for mozilla/embedding that could be shared.

Comment 15

[Simon Fraser [no longer active]]

cc self.

Comment 16

[Conrad Carlen (not reading bugmail)]

The bulk of the code is in common - in nsIFindAndReplace which this code uses.
The rest could be made common if we had Find(), etc. on nsIDOMWindow.

Comment 17

[saari (gone)]

Your implementation looks fine, but nsIWebBrowserFocus (which has IDL checked
in, but I havn't tested the implementation yet) has attribute nsIDOMWindow
focusedWindow; as part of it.

Shouldn't this just live there?
If you want to check this in I can retrofit it when I land my implementation.

Comment 18

[Conrad Carlen (not reading bugmail)]

That is where my GetFocusedWindow() lives. The method was aleady there but empty
on nsWebBrowser::nsIWebBrowserFocus. I just filled it in.  

Comment 19

[saari (gone)]

Doh, sorry, I should pay more attention. Proceed.

Comment 20

[Alec Flett]

looks good. I'm assuming we should switch over to this interface in
mozilla-the-browser as well? Conrad can you file a bug against that?


my one comment on the interface, is why have
void findNext(out boolean didFind);
when you could have
boolean findNext(); ?

makes it a whole lot easier from a scripter's point of view...

Comment 21

[Conrad Carlen (not reading bugmail)]

> I'm assuming we should switch over to this interface in mozilla-the-browser as
> well?
I'm not sure how much the two have in common other than what they do share - use
of the nsIFindAndReplace component. The Find component used in the browser
exists mainly to manage interraction between the JS find window and the find
implementation whereas the point of this one is to have no tie to any UI.

> my one comment on the interface, is why have
> void findNext(out boolean didFind);
> when you could have
> boolean findNext(); ?

Good point - I'll make the change.

Comment 22

[vidur (gone)]

Some nits:
1) In nsWebBrowser::GetFocusedWindow, you do an extra QI - you should be able to
QI directly from nsIDOMWindow to nsPIDOMWindow (we should get rid of the latter
interface anyway - it should get merged with nsIDOMWindowInternal).
2) In nsWebBrowserFindImpl::MakeTSDocument, can't you get the document directly
from the nsIDOMWindow using nsIDOMWindow::GetDocument()?
3) I'm not a big fan of accessors returning references to members (the methods
that allow access to the falgs in nsWebBrowserFindImpl()). Just a personal
preference, I guess.

The first couple of issues should be fixed. Other than that, looks good. sr=vidur.

Comment 23

[Simon Fraser [no longer active]]

> I'm not sure how much the two have in common other than what they do share - use
> of the nsIFindAndReplace component. The Find component used in the browser
> exists mainly to manage interraction between the JS find window and the find
> implementation whereas the point of this one is to have no tie to any UI.

IMO the implemtation of the Find component in Mozilla is really broken. There is 
C++ code that brings up a JS dialog, which seems just back-asswards to me. This 
should be rewritten so that the dialog is invoked from XUL/JS, and just talks to 
the XPIDLsed find interfaces.

Comment 24

[Conrad Carlen (not reading bugmail)]

Incorporated Alec's suggestion about boolean findNext(); as well Vidur's points.

Comment 25

[Alec Flett]

as a side note, simon is right :) you mind filing a bug on the brokeness?

other than that, sr=alecf with that.

Comment 26

[Conrad Carlen (not reading bugmail)]

Yes, I agree with Simon too. I'll file a bug.

Comment 27

[Conrad Carlen (not reading bugmail)]

Fixed checked in.

Comment 28

[Michael Dunn]

Correction: Changing QA contact for the Embed API bugs to David Epstein.

Comment 29

[David Epstein]

find is in embedding apps (mfcembed, testembed).