Closed Bug 46869 Opened 20 years ago Closed 19 years ago

Embedding apps need Find functionality

Categories

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

x86
Windows 98
defect

Tracking

(Not tracked)

VERIFIED FIXED
mozilla0.8

People

(Reporter: jud, Assigned: ccarlen)

References

Details

(Keywords: embed, Whiteboard: [nsbeta3-])

Attachments

(4 files)

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.
Keywords: embed
Keywords: nsbeta3
Whiteboard: [nsbeta3+]
Target Milestone: --- → M18
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.  
Status: NEW → ASSIGNED
Assignee: conrad → ccarlen
Status: ASSIGNED → NEW
changing to new email
re-accepted at new email
Status: NEW → ASSIGNED
Priority: P3 → P1
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.
Whiteboard: [nsbeta3+] → [nsbeta3-]
Target Milestone: M18 → mozilla0.8
Updating QA Contact
QA Contact: jrgm → mdunn
Blocks: 64846
Blocks: 64833
Summary: nsITextServices should not be what we use for find dialog support. → Embedding apps need Find functionality
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. 
Conrad, looks good. r=kin
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.
Chris (Saari) - Can you take a look at the nsWebBrowser::GetFocusedWindow()
 since you're dealing w/ focus issues?
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.
cc self.
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.
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.
That is where my GetFocusedWindow() lives. The method was aleady there but empty
on nsWebBrowser::nsIWebBrowserFocus. I just filled it in.  
Doh, sorry, I should pay more attention. Proceed.
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...
> 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.



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.
> 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.
Incorporated Alec's suggestion about boolean findNext(); as well Vidur's points.
as a side note, simon is right :) you mind filing a bug on the brokeness?

other than that, sr=alecf with that.
Yes, I agree with Simon too. I'll file a bug.

Fixed checked in.
Status: ASSIGNED → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
Correction: Changing QA contact for the Embed API bugs to David Epstein.
QA Contact: mdunn → depstein
find is in embedding apps (mfcembed, testembed).
Status: RESOLVED → VERIFIED
No longer blocks: 64833
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.