Closed
Bug 305763
Opened 19 years ago
Closed 19 years ago
Application control models
Categories
(Core Graveyard :: XForms, enhancement)
Core Graveyard
XForms
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.
Comment 1•19 years ago
|
||
It sounds interesting, but I'm not sure I follow you. Do you have an example application / use case?
Assignee | ||
Comment 2•19 years ago
|
||
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)
Comment 3•19 years ago
|
||
(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...
Comment 4•19 years ago
|
||
(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.
Assignee | ||
Updated•19 years ago
|
Attachment #196517 -
Flags: review?(allan)
Assignee | ||
Comment 5•19 years ago
|
||
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)
Comment 6•19 years ago
|
||
(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()?
Assignee | ||
Comment 7•19 years ago
|
||
Comment on attachment 196915 [details] [diff] [review] v2. I have to solve the problems with lazyInstance too...
Attachment #196915 -
Flags: review?(allan)
Assignee | ||
Updated•19 years ago
|
Attachment #196915 -
Attachment is obsolete: true
Assignee | ||
Comment 8•19 years ago
|
||
Creating _moz_lazy_instance_ element when lazy authoring is used.
Attachment #198241 -
Flags: review?(allan)
Assignee | ||
Comment 9•19 years ago
|
||
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 10•19 years ago
|
||
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+
Assignee | ||
Updated•19 years ago
|
Attachment #198241 -
Flags: review?(aaronr)
Comment 11•19 years ago
|
||
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-
Assignee | ||
Comment 12•19 years ago
|
||
(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.
Assignee | ||
Comment 13•19 years ago
|
||
(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.
Assignee | ||
Comment 14•19 years ago
|
||
(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.
Comment 15•19 years ago
|
||
(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.
Comment 16•19 years ago
|
||
(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
Assignee | ||
Comment 17•19 years ago
|
||
Assignee | ||
Comment 18•19 years ago
|
||
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)
Assignee | ||
Comment 19•19 years ago
|
||
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.
Assignee | ||
Comment 20•19 years ago
|
||
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)
Assignee | ||
Comment 21•19 years ago
|
||
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)
Assignee | ||
Updated•19 years ago
|
Attachment #198593 -
Flags: review?(aaronr)
Assignee | ||
Comment 22•19 years ago
|
||
Attachment #198593 -
Attachment is obsolete: true
Attachment #198596 -
Flags: review?(aaronr)
Assignee | ||
Updated•19 years ago
|
Attachment #198596 -
Flags: review?(aaronr)
Assignee | ||
Comment 23•19 years ago
|
||
Attachment #198665 -
Flags: review?(allan)
Assignee | ||
Comment 24•19 years ago
|
||
Attachment #198667 -
Flags: review?(allan)
Assignee | ||
Comment 25•19 years ago
|
||
put this to components/ directory. You may have to delete compreg.dat (or what it is called..) in your profile.
Assignee | ||
Comment 26•19 years ago
|
||
Needs attachment 198665 [details] [diff] [review], attachment 198667 [details] [diff] [review] and attachment 198668 [details]
Assignee | ||
Updated•19 years ago
|
Attachment #198665 -
Flags: review?(aaronr)
Assignee | ||
Updated•19 years ago
|
Attachment #198667 -
Flags: review?(aaronr)
Comment 27•19 years ago
|
||
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 28•19 years ago
|
||
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.
Assignee | ||
Comment 29•19 years ago
|
||
(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 30•19 years ago
|
||
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+
Comment 31•19 years ago
|
||
(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.
Assignee | ||
Comment 32•19 years ago
|
||
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 33•19 years ago
|
||
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+
Assignee | ||
Comment 34•19 years ago
|
||
(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 35•19 years ago
|
||
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+
Assignee | ||
Comment 36•19 years ago
|
||
Assignee | ||
Updated•19 years ago
|
Attachment #198665 -
Attachment is obsolete: true
Assignee | ||
Updated•19 years ago
|
Attachment #198665 -
Attachment is obsolete: false
Assignee | ||
Updated•19 years ago
|
Attachment #198667 -
Attachment is obsolete: true
Assignee | ||
Comment 37•19 years ago
|
||
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
Assignee | ||
Updated•19 years ago
|
Attachment #198596 -
Attachment is obsolete: true
Comment 38•19 years ago
|
||
What patches here need to go onto the branch?
Comment 39•19 years ago
|
||
Checked in to 1.8 branch
Status: ASSIGNED → RESOLVED
Closed: 19 years ago
Keywords: fixed1.8
Resolution: --- → FIXED
Whiteboard: xf-to-branch
Updated•8 years ago
|
Product: Core → Core Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•