278370 - Restructure access to MDG in nsIModelElementPrivate.idl

Status: RESOLVED FIXED

(Core Graveyard :: XForms, defect)

Description

[Allan Beaufour]

Quoting smaug on bug 265467:
>>+  readonly attribute nsXFormsMDGEngine MDG;
>Hmm, using non-interface class in an interface.

Comment 1

[Allan Beaufour]

Hmmm, there are two reasons the MDG is there:
1)To allow access to Get/SetNodeValue(). These can easily be proxied via
nsIModelElementPrivate. 

2) To allow access to nsXFormsNodeState. This will not be necessary when bug
271720 is fixed, as the control can then access :read-only, :valid, etc.

Comment 2

[Allan Beaufour]

Attachment: Patch v1

I removed the direct MDG access in nsIModelElementPrivate. Instead I let it
proxy set/getNodeValue() for the MDG.

So mMDG is gone from nsXFormsControlStub, but instead I cache a pointer to the
model. We use it a lot. I also added a RemoveFormControl() to its
OnDestroyed().

nsXFormsControlStub also reads the read-only-state of itself from the
attribute, as it should. nsXFormsNodeState should never have been passed to it
in the first place. My bad.

Comment 3

[Olli Pettay [:smaug][[e-mail]] (holiday -> Aug30)]

Comment on attachment 171631 [details] [diff] [review]
Patch v1

 > NS_IMETHODIMP
> nsXFormsUploadElement::Blur(nsIDOMEvent *aEvent)
> {
>-  if (!mInput || mBoundNode || mMDG)
>+  if (!mInput || mBoundNode)

Shouldn't this be: if (!mInput || !mBoundNode || !mModel) ?
Check similar thing also in XFormsInputElement::Blur().

With that r=me

Comment 4

[Allan Beaufour]

(In reply to comment #3)
> (From update of attachment 171631 [details] [diff] [review] [edit])
>  > NS_IMETHODIMP
> > nsXFormsUploadElement::Blur(nsIDOMEvent *aEvent)
> > {
> >-  if (!mInput || mBoundNode || mMDG)
> >+  if (!mInput || mBoundNode)
> 
> Shouldn't this be: if (!mInput || !mBoundNode || !mModel) ?
> Check similar thing also in XFormsInputElement::Blur().

Indeed it should. Sloppy coding from me, sorry.

Comment 5

[Allan Beaufour]

Attachment: Patch v2

+ smaug's comment

Comment 6

[Allan Beaufour]

Attachment: Patch v2 for trunk

Updated patch for trunk.

Comment 7

[Allan Beaufour]

Comment on attachment 171867 [details] [diff] [review]
Patch v2 for trunk

I sense that Darin has enough to do ftm.

Comment 8

[Doron Rosenberg (IBM)]

I am currently using nsIModelElementPrivate::GetMDG() to get at the nodestate of
a domNode (code is not checked in) for submission to check if all required nodes
have a value.  I use he nodestate to check if a node is required, and it seems
this patch will break that.  Is there another way?

Comment 9

[Allan Beaufour]

(In reply to comment #8)
> I am currently using nsIModelElementPrivate::GetMDG() to get at the nodestate of
> a domNode (code is not checked in) for submission to check if all required nodes
> have a value.  I use he nodestate to check if a node is required, and it seems
> this patch will break that.  Is there another way?

argh. Which bug is that? I'll look at it and see.

Comment 10

[Allan Beaufour]

Attachment: Updated to current CVS

Comment 11

[Brian Ryner (not reading)]

Comment on attachment 171867 [details] [diff] [review]
Patch v2 for trunk

Removing review request until allan and doron have worked out access to the MDG
object.

Comment 12

[Doron Rosenberg (IBM)]

actually, Allan an I talked and we agreed that my code can be moved to the Model
code and thus doesn't need getMDG() anymore.

Comment 13

[Allan Beaufour]

(In reply to comment #12)
> actually, Allan an I talked and we agreed that my code can be moved to the Model
> code and thus doesn't need getMDG() anymore.

Doron, could you put the bug with your code on the dep.list for this bug?

Comment 14

[Doron Rosenberg (IBM)]

The bug is bug 279449, not sure if this one depends on it though.

Comment 15

[Allan Beaufour]

Attachment: Updated to current CVS

Doron is not using GetMDG, so here's an up-to-date patch. Rerequesting sr.

Comment 16

[Brian Ryner (not reading)]

Comment on attachment 172792 [details] [diff] [review]
Updated to current CVS

> PRBool
> nsXFormsControlStub::GetReadOnlyState()
> {
>   PRBool res = PR_FALSE;
>-  if (mBoundNode && mMDG) {
>-    const nsXFormsNodeState* ns = mMDG->GetNodeState(mBoundNode);
>-    if (ns) {
>-      res = ns->IsReadonly();
>-    }
>-  }
>+  if (mElement) {
>+    mElement->HasAttribute(NS_LITERAL_STRING("read-only"), &res);
>+  }  

This suggestion isn't specific to this patch, as it would require changing a
couple of other locations... how about changing this to __moz_readonly and
adding a comment that we should not be modifying the document DOM?  Same for
the other attributes.

I'm worried that if we ship a beta release with these attributes that people
will begin using them and we'll never be able to get rid of them in favor of
the CSS pseudoclasses.

Anyway, sr=me for this patch since you're not introducing new attributes or
anything.

Comment 17

[Allan Beaufour]

(In reply to comment #16)
> This suggestion isn't specific to this patch, as it would require changing a
> couple of other locations... how about changing this to __moz_readonly and
> adding a comment that we should not be modifying the document DOM?  Same for
> the other attributes.
> 
> I'm worried that if we ship a beta release with these attributes that people
> will begin using them and we'll never be able to get rid of them in favor of
> the CSS pseudoclasses.

Good point, Or rather __moz_TEMPORARY_readonly, or something _really_ obvious?

> Anyway, sr=me for this patch since you're not introducing new attributes or
> anything.

Ok thanks, I'll check it in later.

Comment 18

[Allan Beaufour]

Checked in.

Also removed bug 271720 dependency ... what was it doing there?