Closed Bug 305763 Opened 19 years ago Closed 19 years ago

Application control models

Categories

(Core Graveyard :: XForms, enhancement)

enhancement
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: smaug, Assigned: smaug)

Details

(Keywords: fixed1.8)

Attachments

(7 files, 6 obsolete files)

27.77 KB, patch
allan
: review+
aaronr
: review-
Details | Diff | Splinter Review
1.03 KB, application/xhtml+xml
Details
1.46 KB, application/xhtml+xml
Details
42.37 KB, patch
allan
: review+
aaronr
: review+
Details | Diff | Splinter Review
3.42 KB, application/x-javascript
Details
662 bytes, application/xhtml+xml
Details
5.89 KB, patch
Details | Diff | Splinter Review
I had an idea to use XForms to control an application, 
for example browser. For that I would have to create an application control
model language (ACML or something :) ), XTF could be used to implement that.
The datamodel would consist of 'ACML' elements, which could be modified
using XForms. But the application should be able to modify the datamodel
too and refresh the UI bound to it. For that it is needed that one can access
the main document from instance data document. So I guess I'd like
to add a property to the instance data document. The property would be 
a pointer to the xf:instance element in the main document.

This all is not quite clear to me yet.
It is just something I've been thinking about and earlier I thought 
I'd have add support for a new URI scheme, but using 'ACML' this all
should be easier to implement.
It sounds interesting, but I'm not sure I follow you. Do you have an example
application / use case?
I think this is what I need at some point.

instanceDocumentOwner makes it easier to create new XML languages, which
are meant to be used in instance data documents. Using the owner property
one can check that the document is still bound to an instance element and
QIing the property to nsIDOMNode and asking the parent node one can get 
access to the model and call re(build|calculate|validate|fresh).

I really should write an example ACML element. ;)
Attachment #196517 - Flags: review?(allan)
(In reply to comment #2)

Ah, now I get it. We have a mechanism to go instance element -> instance
document, you need instance document -> instance element. Right?

> Created an attachment (id=196517) [edit]
> Add instanceDocumentOwner property

Do you not need it to be exposed? scriptable?

It's to bad that DOMDocument cannot have an ownerDocument, that could have been
a way to do it. But it might also just have been ugly...
(In reply to comment #3)
> (In reply to comment #2)
> 
> Ah, now I get it. We have a mechanism to go instance element -> instance
> document, you need instance document -> instance element. Right?

If I understand you correctly, this would also mean that
nsXFormsUtils::GetInstanceNodeForData() could be done a lot better than now.
Attachment #196517 - Flags: review?(allan)
Attached patch v2. (obsolete) — Splinter Review
Using instanceDocumentOwner in nsXFormsUtils::GetInstanceNodeForData().

And btw, in nsXFormsInsertDeleteElement::HandleAction the model element
is needed elsewhere (it doesn't show in the patch), so I left the 
line nsCOMPtr<nsIDOMElement> modelElem = do_QueryInterface(model);
Attachment #196517 - Attachment is obsolete: true
Attachment #196915 - Flags: review?(allan)
(In reply to comment #5)
> Created an attachment (id=196915) [edit]
> v2.
> 
> Using instanceDocumentOwner in nsXFormsUtils::GetInstanceNodeForData().
> 
> And btw, in nsXFormsInsertDeleteElement::HandleAction the model element
> is needed elsewhere (it doesn't show in the patch), so I left the 
> line nsCOMPtr<nsIDOMElement> modelElem = do_QueryInterface(model);

Shouldn't you update the property on nsXFormsInstanceElement::SetDocument()?
Comment on attachment 196915 [details] [diff] [review]
v2.

I have to solve the problems with lazyInstance too...
Attachment #196915 - Flags: review?(allan)
Attachment #196915 - Attachment is obsolete: true
Attached patch v3Splinter Review
Creating _moz_lazy_instance_ element when lazy authoring is used.
Attachment #198241 - Flags: review?(allan)
This is not exactly a testcase for this bug, but it shows what can be
done with lazy instances.
Needs the fix in Bug 310840.
Comment on attachment 198241 [details] [diff] [review]
v3

Tricky business with lazy instances -- the "_moz_lazy..." hurts my eyes, but
it's necessary.

> nsXFormsUtils::GetInstanceNodeForData(nsIDOMNode             *aInstanceDataNode,
>-                                      nsIModelElementPrivate *aModel,
>                                       nsIDOMNode             **aInstanceNode)
> {
>   NS_ENSURE_ARG(aInstanceDataNode);
>-  NS_ENSURE_ARG(aModel);
>   NS_ENSURE_ARG_POINTER(aInstanceNode);
>   *aInstanceNode = nsnull;
> 
>-  /* We want to get at the <xf:instance> that aInstanceDataNode belongs to.
>-     We get all xf:instance nodes in the aModel, QI it to nsIInstanceElementPrivate
>-     and compare its document to the document aInstanceDataNode lives in.
>-   */
>-
>-  nsCOMPtr<nsIDOMDocument> instanceDoc;
>-  aInstanceDataNode->GetOwnerDocument(getter_AddRefs(instanceDoc));
>+  nsCOMPtr<nsIDOMDocument> instanceDOMDoc;
>+  aInstanceDataNode->GetOwnerDocument(getter_AddRefs(instanceDOMDoc));
>   // owner doc is null when the data node is the document (e.g., ref="/")
>-  if (!instanceDoc)
>-    instanceDoc = do_QueryInterface(aInstanceDataNode);
>-  NS_ENSURE_TRUE(instanceDoc, NS_ERROR_UNEXPECTED);
>-
>-  nsCOMArray<nsIInstanceElementPrivate> *instList = nsnull;
>-  aModel->GetInstanceList(&instList);
>-  NS_ENSURE_TRUE(instList, NS_ERROR_FAILURE);
>+  if (!instanceDOMDoc) {
>+    instanceDOMDoc = do_QueryInterface(aInstanceDataNode);
>+  }

This happens if aInstanceDataNode is the document node, right? Please add a
comment about that, or whatever it's for if I'm wrong :)


r=me
Attachment #198241 - Flags: review?(allan) → review+
Attachment #198241 - Flags: review?(aaronr)
Comment on attachment 198241 [details] [diff] [review]
v3

>Index: nsXFormsAtoms.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/extensions/xforms/nsXFormsAtoms.cpp,v
>retrieving revision 1.15
>diff -u -8 -p -r1.15 nsXFormsAtoms.cpp
>--- nsXFormsAtoms.cpp	16 Sep 2005 10:53:36 -0000	1.15
>+++ nsXFormsAtoms.cpp	2 Oct 2005 18:28:26 -0000
>@@ -59,16 +59,17 @@ nsIAtom *nsXFormsAtoms::selected;
> nsIAtom *nsXFormsAtoms::appearance;
> nsIAtom *nsXFormsAtoms::incremental;
> nsIAtom *nsXFormsAtoms::clazz;
> nsIAtom *nsXFormsAtoms::deferredBindListProperty;
> nsIAtom *nsXFormsAtoms::readyForBindProperty;
> nsIAtom *nsXFormsAtoms::accesskey;
> nsIAtom *nsXFormsAtoms::fatalError;
> nsIAtom *nsXFormsAtoms::isInstanceDocument;
>+nsIAtom *nsXFormsAtoms::instanceDocumentOwner;

If we have instanceDocumentOwner now, do we really need isInstanceDocument
any more?  Though I do like the readibility of isInstanceDocument.

>Index: nsXFormsInstanceElement.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/extensions/xforms/nsXFormsInstanceElement.cpp,v
>retrieving revision 1.18
>diff -u -8 -p -r1.18 nsXFormsInstanceElement.cpp
>--- nsXFormsInstanceElement.cpp	26 Sep 2005 14:21:52 -0000	1.18
>+++ nsXFormsInstanceElement.cpp	2 Oct 2005 18:28:27 -0000

> NS_IMETHODIMP
> nsXFormsInstanceElement::SetDocument(nsIDOMDocument *aDocument)
> {
>+  nsCOMPtr<nsIDocument> doc(do_QueryInterface(mDocument));
>+  if (doc) {
>+    doc->UnsetProperty(nsXFormsAtoms::instanceDocumentOwner);
>+  }
>+
>   mDocument = aDocument;
>+
>+  doc = do_QueryInterface(mDocument);
>+  if (doc) {
>+    // Set property to prevent an instance document loading an external instance
>+    // document
>+    nsresult rv = doc->SetProperty(nsXFormsAtoms::isInstanceDocument, doc);
>+    NS_ENSURE_SUCCESS(rv, rv);
>+
>+    nsCOMPtr<nsISupports> owner(do_QueryInterface(mElement));
>+    NS_ENSURE_STATE(owner);
>+    rv = doc->SetProperty(nsXFormsAtoms::instanceDocumentOwner, owner);
>+    NS_ENSURE_SUCCESS(rv, rv);
>+  }
>+
>   return NS_OK;
> }
> 

Kinda ties in with my first comment.  instanceDocumentOwner and
isInstanceDocument
shouldn't be independent of each other (really doesn't make sense to have one 
set without the other being set).  Here, it looks like if aDocument is nsnull,  
you'll unset the instanceDocumentOwner property but not the isInstanceDocument
property


>Index: nsXFormsLazyInstanceElement.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/extensions/xforms/nsXFormsLazyInstanceElement.cpp,v
>retrieving revision 1.1
>diff -u -8 -p -r1.1 nsXFormsLazyInstanceElement.cpp
>--- nsXFormsLazyInstanceElement.cpp	29 Jun 2005 18:14:43 -0000	1.1
>+++ nsXFormsLazyInstanceElement.cpp	2 Oct 2005 18:28:27 -0000

> NS_IMETHODIMP
> nsXFormsLazyInstanceElement::SetDocument(nsIDOMDocument *aDocument)
> {
>+  nsCOMPtr<nsIDocument> doc(do_QueryInterface(mDocument));
>+  if (doc) {
>+    doc->UnsetProperty(nsXFormsAtoms::instanceDocumentOwner);
>+  }
>+
>   mDocument = aDocument;
>+
>+  doc = do_QueryInterface(mDocument);
>+  if (doc) {
>+    // Set property to prevent an instance document loading an external instance
>+    // document
>+    nsresult rv = doc->SetProperty(nsXFormsAtoms::isInstanceDocument, doc);
>+    NS_ENSURE_SUCCESS(rv, rv);
>+
>+    nsCOMPtr<nsISupports> owner(do_QueryInterface(mElement));
>+    NS_ENSURE_STATE(owner);
>+    rv = doc->SetProperty(nsXFormsAtoms::instanceDocumentOwner, owner);
>+    NS_ENSURE_SUCCESS(rv, rv);
>+  }
>+
>   return NS_OK;
> }
> 

Same here.

I guess that I don't really mind which way you go...either fixing the
::SetDocument
functions or removing isInstanceDocument, I don't really mind either way.

Ugh, I really don't like the processor adding anything to the DOM.  Can you
explain
a little more why we need xf:_moz_lazy_instance_?  Add comments to
nsXFormsLazyInstanceElement?
Thanks.
Attachment #198241 - Flags: review?(aaronr) → review-
(In reply to comment #11)
> 
> If we have instanceDocumentOwner now, do we really need isInstanceDocument
> any more?  Though I do like the readibility of isInstanceDocument.
> 


Yes, we do need isInstanceDocument AND instanceDocumentOwner.
The instance document may not be bound to any xf:intance element anymore, 
but it should still know that it is really an instance document - 
just to make sure that it doesn't start to load any new instances (if the
instance document contains xf:instance elements). isInstanceDocument 
will be removed from the document when the document is deleted.

On the other hand instanceDocumentOwner is set to document iff
the document is owned by an xf:instance element.
instanceDocumentOwner is removed from the document when the xf:instance
element loses the pointer to the document.
(In reply to comment #11)
> Ugh, I really don't like the processor adding anything to the DOM.  Can you
> explain
> a little more why we need xf:_moz_lazy_instance_?  Add comments to
> nsXFormsLazyInstanceElement?
> 

I don't like that either but how would you support 
xforms-insert and xforms-delete events with lazy instances?
An event listener listening those events in <xf:model> should be able
to capture them.

(In reply to comment #11)
> Ugh, I really don't like the processor adding anything to the DOM.  Can you
> explain
> a little more why we need xf:_moz_lazy_instance_?  Add comments to
> nsXFormsLazyInstanceElement?
> Thanks.
> 

I got a new idea for this. If it works we will able to supports events
(almost) properly, but still leave the lazy element out of DOM.
I'll test it asap.
(In reply to comment #13)
> (In reply to comment #11)
> > Ugh, I really don't like the processor adding anything to the DOM.  Can you
> > explain
> > a little more why we need xf:_moz_lazy_instance_?  Add comments to
> > nsXFormsLazyInstanceElement?
> > 
> 
> I don't like that either but how would you support 
> xforms-insert and xforms-delete events with lazy instances?
> An event listener listening those events in <xf:model> should be able
> to capture them.
> 
> 

Honestly, I would say that we shouldn't fire those events at lazy authored
instance elements.  xforms-insert and xforms-delete are targeted at
xforms:instance elements.  Even with your patch, lazy authored instance elements
aren't xf:instance elements.  I'll try this out on other processors and see what
they do.
(In reply to comment #15)
> (In reply to comment #13)
> > (In reply to comment #11)
> > > Ugh, I really don't like the processor adding anything to the DOM.  Can you
> > > explain
> > > a little more why we need xf:_moz_lazy_instance_?  Add comments to
> > > nsXFormsLazyInstanceElement?
> > > 
> > 
> > I don't like that either but how would you support 
> > xforms-insert and xforms-delete events with lazy instances?
> > An event listener listening those events in <xf:model> should be able
> > to capture them.
> > 
> > 
> 
> Honestly, I would say that we shouldn't fire those events at lazy authored
> instance elements.  xforms-insert and xforms-delete are targeted at
> xforms:instance elements.  Even with your patch, lazy authored instance elements
> aren't xf:instance elements.  I'll try this out on other processors and see what
> they do.


Hmmm, the spec does say that a "default instance is created", so we probably
should generate the events
Attached patch alternative proposal (obsolete) — Splinter Review
with this an <instance> element is actually created, but it is not put
to document. When dispatching events to it, the target is the parent
<model> and originalTarget is <instance>.
Attachment #198486 - Flags: review?(aaronr)
attachment 198241 [details] [diff] [review] and attachment 198486 [details] [diff] [review] are both hacks, 
but I can blame XForms specification.
Lazy instances aren't just well enough defined.

*I think* I prefer attachment 198486 [details] [diff] [review], that way we don't have to
modify DOM tree.
Comment on attachment 198486 [details] [diff] [review]
alternative proposal

hopefully a better patch coming. I'll put the lazy instance element to
anonymous DOM. AddBinding should work nowadays.
Attachment #198486 - Flags: review?(aaronr)
Attached patch lazy instance in anonymous DOM (obsolete) — Splinter Review
This is using an XBL binding for lazy instances.
nsXFormsLazyInstanceElement.* are not needed anymore, I merged
the functionality to nsXFormsInstanceElement.*

Btw, the lazy <xf:instance> can be seen in DOM Inspector.
Attachment #198486 - Attachment is obsolete: true
Attachment #198593 - Flags: review?(aaronr)
Attachment #198593 - Flags: review?(aaronr)
Attached patch oops, this is the patch (obsolete) — Splinter Review
Attachment #198593 - Attachment is obsolete: true
Attachment #198596 - Flags: review?(aaronr)
Attachment #198596 - Flags: review?(aaronr)
put this to components/ directory.
You may have to delete compreg.dat (or what it is called..) in your profile.
Attachment #198665 - Flags: review?(aaronr)
Attachment #198667 - Flags: review?(aaronr)
Comment on attachment 198665 [details] [diff] [review]
Fixing one warning related normal instance elements

nice job.  r=me
Attachment #198665 - Flags: review?(aaronr) → review+
Comment on attachment 198667 [details] [diff] [review]
Additional patch to make it possible to access instanceDocumentOwner from JS

I see how this is the best way to do what you need to do for this one thing. 
But if you see more 'features' coming down the road with applications being
built in the XForms instance document, I'd rather not do something like this
for each feature.    If you think that this could be big, maybe we ought to
think about extending nsIDocument and then providing a supported interface
which can find out this type of information.
(In reply to comment #28)
> (From update of attachment 198667 [details] [diff] [review] [edit])
> I see how this is the best way to do what you need to do for this one thing. 
> But if you see more 'features' coming down the road with applications being
> built in the XForms instance document, I'd rather not do something like this
> for each feature.    

I don't have any other 'features' in my mind now ;)


> If you think that this could be big, maybe we ought to
> think about extending nsIDocument and then providing a supported interface
> which can find out this type of information.
> 

No idea if this could be big, although I can imagine many use cases.
Unfortunately I don't see any way how an extension could extend nsDocument.
XPath is a special case:
http://lxr.mozilla.org/seamonkey/source/content/base/src/nsDocument.cpp#839
Comment on attachment 198665 [details] [diff] [review]
Fixing one warning related normal instance elements

>Index: nsXFormsInstanceElement.cpp
>===================================================================

>+NS_IMETHODIMP
>+nsXFormsInstanceElement::InitializeLazyInstance()
>+  // I don't know if not being able to create a backup document is worth
>+  // failing this function.  Since it probably won't be used often, we'll
>+  // let it slide.  But it probably does mean that things are going south
>+  // with the browser.
>+  domImpl->CreateDocument(EmptyString(), EmptyString(), nsnull,
>+                          getter_AddRefs(mOriginalDocument));

Please emit a warning then? I foresee debugging this will be a pain if there's
no warning.

>Index: nsXFormsModelElement.cpp
>===================================================================

>@@ -410,22 +413,48 @@ nsXFormsModelElement::DoneAddingChildren
> 
>   // If all of the children are added and there aren't any instance elements,
>   // yet, then we need to make sure that one is ready in case the form author
>   // is using lazy authoring.
>   PRUint32 instCount = mInstanceList.Count();
>   if (!instCount) {
>     nsCOMPtr<nsIDOMDocument> domDoc;
>     mElement->GetOwnerDocument(getter_AddRefs(domDoc));
>-    if (domDoc) {
>-      nsXFormsLazyInstanceElement *lazyInstance = 
>-                                             new nsXFormsLazyInstanceElement();
>-      lazyInstance->CreateLazyInstanceDocument(domDoc);
>-      AddInstanceElement(lazyInstance);
>-      mLazyModel = PR_TRUE;
>+    nsCOMPtr<nsIDOMDocumentXBL> xblDoc(do_QueryInterface(domDoc));
>+    if (xblDoc) {
>+      nsresult rv =
>+        xblDoc->AddBinding(mElement,
>+                           NS_LITERAL_STRING(XFORMS_LAZY_INSTANCE_BINDING));
>+      NS_ENSURE_SUCCESS(rv, rv);
>+
>+      NS_WARN_IF_FALSE(mInstanceList.Count() == 1, 
>+                       "Installing lazy instance didn't succeed!");
>+
>+      nsCOMPtr<nsIDOMNodeList> list;
>+      xblDoc->GetAnonymousNodes(mElement, getter_AddRefs(list));
>+      if (list) {
>+        PRUint32 childCount = 0;
>+        if (list) {
>+          list->GetLength(&childCount);
>+        }
>+
>+        for (PRUint32 i = 0; i < childCount; ++i) {
>+          nsCOMPtr<nsIDOMNode> item;
>+          list->Item(i, getter_AddRefs(item));
>+          nsCOMPtr<nsIInstanceElementPrivate> instance =
>+            do_QueryInterface(item);
>+          if (instance) {
>+            rv = instance->InitializeLazyInstance();
>+            NS_ENSURE_SUCCESS(rv, rv);
>+
>+            mLazyModel = PR_TRUE;
>+            break;
>+          }
>+        }
>+      }
>     }
>   }

If I understand this correctly, then you bind XBL to the model which creates an
instance element as its anonymous content? Not too shaby a solution. Good
thinking.

But please include a comment somwhere, in plain text, that explains this.

r=me with the warning and the comment
Attachment #198665 - Flags: review?(allan) → review+
(In reply to comment #29)
> (In reply to comment #28)
> > (From update of attachment 198667 [details] [diff] [review] [edit] [edit])
> > I see how this is the best way to do what you need to do for this one thing. 
> > But if you see more 'features' coming down the road with applications being
> > built in the XForms instance document, I'd rather not do something like this
> > for each feature.    
> 
> I don't have any other 'features' in my mind now ;)
> 
> 
> > If you think that this could be big, maybe we ought to
> > think about extending nsIDocument and then providing a supported interface
> > which can find out this type of information.
> > 
> 
> No idea if this could be big, although I can imagine many use cases.
> Unfortunately I don't see any way how an extension could extend nsDocument.
> XPath is a special case:
> http://lxr.mozilla.org/seamonkey/source/content/base/src/nsDocument.cpp#839


If we can't override document, then maybe we should override nsIDOMElement with
some functions added to the interface to do this 'application' type of work? 
Then make sure that the root element of the instance document is of this new
class?  Having to set up a 'feature' and version number for every
application-type method just seems like overkill.
Checked in attachment 198665 [details] [diff] [review] (+review comments).
Leaving open for branch and possible additional patch for JS.
Status: NEW → ASSIGNED
Whiteboard: xf-to-branch
Comment on attachment 198667 [details] [diff] [review]
Additional patch to make it possible to access instanceDocumentOwner from JS

The patch is fine, but I would rather see a name like
org.mozilla.xforms.instanceOwner to keep the same naming conventions. So with
something in that direction, r=me

(sorry for the delay, I missed the request)
Attachment #198667 - Flags: review?(allan) → review+
(In reply to comment #33)
> (From update of attachment 198667 [details] [diff] [review] [edit])
> The patch is fine, but I would rather see a name like
> org.mozilla.xforms.instanceOwner to keep the same naming conventions.

Aaron, would this be ok to you?

Comment on attachment 198667 [details] [diff] [review]
Additional patch to make it possible to access instanceDocumentOwner from JS

echoing Allan's comment about name change.  Rest is ok. With that, r=me
Attachment #198667 - Flags: review?(aaronr) → review+
Attachment #198665 - Attachment is obsolete: true
Attachment #198665 - Attachment is obsolete: false
Attachment #198667 - Attachment is obsolete: true
Comment on attachment 200139 [details] [diff] [review]
Additional patch to make it possible to access instanceDocumentOwner from JS (with the right feature name)

checked in
Attachment #198596 - Attachment is obsolete: true
What patches here need to go onto the branch?
Checked in to 1.8 branch
Status: ASSIGNED → RESOLVED
Closed: 19 years ago
Keywords: fixed1.8
Resolution: --- → FIXED
Whiteboard: xf-to-branch
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: