74922 - Need to be able to hang editors off of docShells

Status: RESOLVED FIXED

(Core :: DOM: Navigation, defect, P3)

Description

[Simon Fraser [no longer active]]

Editor needs to be able to hang editor data off each docShell, so that we can 
handle editing framesets, iframes etc. DocShell will hold the owning reference to 
an editor on each editable subframe. There will be a top-level nsIEditingSession 
(implemented in composer code) which clients use to address specific editors.

Patches coming.

Comment 1

[Simon Fraser [no longer active]]

Attachment: nsDocShellEditorData.cpp

Comment 2

[Simon Fraser [no longer active]]

Attachment: nsDocShellEditorData.h

Comment 3

[Simon Fraser [no longer active]]

Attachment: nsIEditorDocShell.idl

Comment 4

[Simon Fraser [no longer active]]

Attachment: Diffs in mozilla/docshell (excluding new files)

Comment 5

[Simon Fraser [no longer active]]

So here's how this works.

nsDocShell implements a new interface, nsIEditorDocShell, which
is simply a private interface (should it be nsPIEditorDocShell?)
which allows you to set a flag on a docShell to make it editable
(either now, or when a URL loads), to get an nsIEditor from
that docShell, and to test if it has an nsIEditingSession (which
will only be the case for the content-root docShell).
You get to a nsIEditorDocShell by doing a QI from nsIDocShell.

nsDocShell internally stores editor-related data in a C++
object, nsDocShellEditorData, which is only allocated when
a docShell has been made editable. Thus, I'm introducing just
an extra 4-bytes per docShell of new overhead. nsDocShellEditorData
contains the 'editable' flag, and nsCOMPtrs to an nsIEditingSession
and/or nsIEditor. These are owning references. nsDocShell's
implementation of nsIEditorDocShell mostly just passes the calls
through to nsDocShellEditorData.

nsDocShell also now supports GetInterface() of an nsIEditingSession,
which is the interface that clients will use to actually make
frames editable (using nsIDOMWindows to refer to frames). You
can see nsIEditingSession at:

<http://lxr.mozilla.org/seamonkey/source/editor/composer/public/
nsIEditingSession.idl>

and the implementation at:

<http://lxr.mozilla.org/seamonkey/source/editor/composer/src/
nsEditingSession.cp>

It is intended that nsIEditingSession will be a public interface.

I'm looking for r= and sr= on the mozilla/docshell changes.

Comment 6

[Judson Valeski]

nsIEditorDocShell.idl is fine, no need for 'P'.

nsIEditingSession.idl -
* predef of nsIEditingShell isn't needed.
* drags in nsIEditor. do we need to make nsIEditor public? If so, we need to
clean it up quite a bit (idl'ize, enums -> java style enums -> remove
dependencies on non-public ifaces, etc). Another option would be to break
nsIEditor apart into internal stuff we don't need (nsIEditorInternal), and just
the stuff we want to expose (nsIEditor). That's a common practice that
elleviates some refactoring.

Comment 7

[kinmoz]

FYI mjudge will be landing some changes that XPIDL'ize most of the low-level
editor interfaces including nsIEditor.

Comment 8

[rubydoo123]

so -- where do we stand on this bug? Jud are you r= on this one? Simon -- who 
should sr= this one?

Comment 9

[Judson Valeski]

well, nsIEditor can't go public as it is (idl'ized or not), should I file a
separate bug for that? r=valeski on the nsIEditingSession.idl after the
nsIEditorShell iface predef is pulled. I'm inclined not to even r= that though
as it drags in nsIEditor in it's dirty form, but if we want to break that into
another bug I'm ok w/ the r=.

the docshell mods look good to me, r=valeski on those. I can't speak for the
editorData stuff though.

Comment 10

[Simon Fraser [no longer active]]

Can we just say that nsIEditingSession is not a public interface for now, and 
bless it and nsIEditor together later?

Comment 11

[Judson Valeski]

sure can.

Comment 12

[Simon Fraser [no longer active]]

So do I have an r= on the nsIEditorSession stuff, with the proviso that it is 
currently not public,  but it and nsIEditor will become so later?

Comment 13

[Judson Valeski]

r=valeski

Comment 14

[Simon Fraser [no longer active]]

-> 0.9.1

Comment 15

[Adam Lock]

I have a few small points but r=[e-mail] anyway for everything:

1. nsDocShellEditorData.cpp says 4 space indentation in the comment, but it's
   really using 2 spaces
2. nsDocShellEditorData::SetEditor() is comparing interface pointers. Is there
   any chance this might fail for example if one of the objects were actually
   a proxy?
3. The docshell diff has some code below a comment that says
   "delete any editor data". It doesn't appear to test if mEditorData is
   nsnull or not before delete'ing it. Is this right?

Comment 16

[Simon Fraser [no longer active]]

> I have a few small points but r=[e-mail] anyway for everything:
> 
> 1. nsDocShellEditorData.cpp says 4 space indentation in the comment, but it's
>    really using 2 spaces

Will fix.

> 2. nsDocShellEditorData::SetEditor() is comparing interface pointers. Is there
>    any chance this might fail for example if one of the objects were actually
>    a proxy?

It could, I guess, tho I don't know of any code that makes proxies to editors.
Would I have to implement an "IsSelf(nsIEditor* inEditor)" method to do this 
properly? This seems like overkill.

> 3. The docshell diff has some code below a comment that says
>    "delete any editor data". It doesn't appear to test if mEditorData is
>    nsnull or not before delete'ing it. Is this right?

C++ standard says that |delete| is safe to call on a null pointer. The lack of a 
null check is deliberate.

Comment 17

[Adam Lock]

For 2) I don't you should worry too much if you know that the nsIEditor stuff 
will happen on the same thread as the docshell and that there are no proxy 
wrappers for anything such as XPConnect.

Comment 18

[rubydoo123]

1.0

Comment 19

[Asa Dotzler [:asa]]

Bugs targeted at mozilla1.0 without the mozilla1.0 keyword moved to mozilla1.0.1 
(you can query for this string to delete spam or retrieve the list of bugs I've 
moved)

Comment 20

[Simon Fraser [no longer active]]

->mjudge

Comment 21

[Mike Lee]

Someone want fix up the target milestone to 1.0 ? since 66296 is target at 1.0
and this is a dependant bug.

Comment 22

[Simon Fraser [no longer active]]

I think this was probably checked in on Friday

Comment 23

[Simon Fraser [no longer active]]

This landed with the editor embedding stuff.