Last Comment Bug 289434 - xbl-ized widgets for xforms
: xbl-ized widgets for xforms
Status: RESOLVED FIXED
:
Product: Core Graveyard
Classification: Graveyard
Component: XForms (show other bugs)
: Trunk
: x86 Windows XP
: -- normal (vote)
: ---
Assigned To: Olli Pettay [:smaug] (vacation Aug 25-28)
: Stephen Pride
Mentors:
Depends on: 291026
Blocks:
  Show dependency treegraph
 
Reported: 2005-04-07 07:35 PDT by Andrew Douglas
Modified: 2016-07-15 14:46 PDT (History)
6 users (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
initial patch for input, output and trigger (66.71 KB, patch)
2005-04-07 07:57 PDT, Andrew Douglas
no flags Details | Diff | Splinter Review
trigger test case. (1.93 KB, application/octet-stream)
2005-04-07 08:00 PDT, Andrew Douglas
no flags Details
second rough draft (70.80 KB, patch)
2005-04-07 22:44 PDT, Andrew Douglas
no flags Details | Diff | Splinter Review
updated to current CVS (137.92 KB, patch)
2005-04-08 09:43 PDT, Allan Beaufour
no flags Details | Diff | Splinter Review
update based on XTFBindable (160.39 KB, patch)
2005-04-21 11:05 PDT, Andrew Douglas
no flags Details | Diff | Splinter Review
updated test case. (1.67 KB, application/zip)
2005-04-21 11:45 PDT, Andrew Douglas
no flags Details
removed some useless files from the patch (116.84 KB, patch)
2005-04-21 11:50 PDT, Andrew Douglas
no flags Details | Diff | Splinter Review
Few fixes, cleaning up things (84.29 KB, patch)
2005-04-23 14:20 PDT, Olli Pettay [:smaug] (vacation Aug 25-28)
no flags Details | Diff | Splinter Review
updated test case, using "inline" xbl (3.52 KB, text/xml)
2005-04-23 14:23 PDT, Olli Pettay [:smaug] (vacation Aug 25-28)
no flags Details
Label should work now inside repeats (86.97 KB, patch)
2005-04-25 14:27 PDT, Olli Pettay [:smaug] (vacation Aug 25-28)
no flags Details | Diff | Splinter Review
<output> inside <repeat> or <message> works. (92.08 KB, patch)
2005-04-26 12:25 PDT, Olli Pettay [:smaug] (vacation Aug 25-28)
no flags Details | Diff | Splinter Review
XBL widgets initialized properly (without using a timer) (94.61 KB, patch)
2005-04-27 12:36 PDT, Olli Pettay [:smaug] (vacation Aug 25-28)
no flags Details | Diff | Splinter Review
works with the latest patch in bug 291026 (75.86 KB, patch)
2005-05-12 11:48 PDT, Olli Pettay [:smaug] (vacation Aug 25-28)
no flags Details | Diff | Splinter Review
setfocus testcase (1.42 KB, application/xhtml+xml)
2005-05-26 15:04 PDT, aaronr
no flags Details
testcase for ephemeral message (1.39 KB, application/xhtml+xml)
2005-05-26 15:07 PDT, aaronr
no flags Details
for review (82.40 KB, patch)
2005-06-05 14:24 PDT, Olli Pettay [:smaug] (vacation Aug 25-28)
no flags Details | Diff | Splinter Review
Testcase: Output with invalid binding (658 bytes, application/xhtml+xml)
2005-06-06 07:20 PDT, Allan Beaufour
no flags Details
Testcase: Dynamic insertion of output (1.30 KB, application/xhtml+xml)
2005-06-06 07:25 PDT, Allan Beaufour
no flags Details
for review 2 (84.99 KB, patch)
2005-06-06 12:53 PDT, Olli Pettay [:smaug] (vacation Aug 25-28)
allan: review-
Details | Diff | Splinter Review
Testcase: Listening for xforms-ready (869 bytes, application/xhtml+xml)
2005-06-07 02:31 PDT, Allan Beaufour
no flags Details
for review 3 (85.24 KB, patch)
2005-06-09 10:46 PDT, Olli Pettay [:smaug] (vacation Aug 25-28)
allan: review-
Details | Diff | Splinter Review
for review 4 (88.40 KB, patch)
2005-06-14 15:10 PDT, Olli Pettay [:smaug] (vacation Aug 25-28)
no flags Details | Diff | Splinter Review
for review 4 (up-to-date) (92.96 KB, patch)
2005-06-20 13:49 PDT, Olli Pettay [:smaug] (vacation Aug 25-28)
allan: review+
doronr: review+
Details | Diff | Splinter Review

Description Andrew Douglas 2005-04-07 07:35:17 PDT
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b2) Gecko/20050305
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b2) Gecko/20050305

We should use xbl for defining the visual content of the xforms widgets and
leave the xtf elements to just define the functionality (such as bindings,
etc.). See http://lab.cph.novell.com/~abeaufour/xforms/uidesign.html

Reproducible: Always
Comment 1 Andrew Douglas 2005-04-07 07:57:21 PDT
Created attachment 179950 [details] [diff] [review]
initial patch for input, output and trigger
Comment 2 Andrew Douglas 2005-04-07 08:00:45 PDT
Created attachment 179951 [details]
trigger test case.

a bit of a hacked test case.. for some reason getElementById was causing me
problems.. not sure if it was just because it was late or if we have an actual
issue.
Comment 3 Andrew Douglas 2005-04-07 22:44:37 PDT
Created attachment 180015 [details] [diff] [review]
second rough draft

still just input, output and trigger. This is mostly cleanup, fixing where the
labels show up in the xbl contents, output value works (right now it's an ugly
refresh that should probably go into the delegate.. but I don't know where),
and the first attempt at getting context working. It works AFTER the first
refresh on the page.. so it's probably something is just not set up when it
first attempts to getcontext. Note: repeats still have issues so make sure to
test outside of a <repeat>
Suggestions for improvements welcome. I hope to get label xbl-ized next and
then deal with repeat.

Other issues to be wary of: ID and class have issues... also
psuedo-elements/classes. I'm sure there are a few others.
Comment 4 Allan Beaufour 2005-04-08 09:43:20 PDT
Created attachment 180065 [details] [diff] [review]
updated to current CVS
Comment 5 Andrew Douglas 2005-04-21 11:05:07 PDT
Created attachment 181437 [details] [diff] [review]
update based on XTFBindable

This is based on the patch for XTFBindableElement in bug 291026. This fixes
context issues, repeat issues, class/id issues and generally just works. Label
is also now xbl-ized. This is pretty much the final form but just requires some
cleanup and then I'll ask for review. If there are any major gotcha's, please
let me know.
-Andrew
Comment 6 Andrew Douglas 2005-04-21 11:45:36 PDT
Created attachment 181444 [details]
updated test case.

We do apparently have an issue with user-defined xbl bindings. The
appearance="beaufour" isn't using the css defined -moz-binding. This may be an
issue with the XTFBindableElement patch.
Comment 7 Andrew Douglas 2005-04-21 11:50:05 PDT
Created attachment 181445 [details] [diff] [review]
removed some useless files from the patch

sorry for all the spam
Comment 8 Olli Pettay [:smaug] (vacation Aug 25-28) 2005-04-23 14:20:44 PDT
Created attachment 181662 [details] [diff] [review]
Few fixes, cleaning up things
Comment 9 Olli Pettay [:smaug] (vacation Aug 25-28) 2005-04-23 14:23:28 PDT
Created attachment 181663 [details]
updated test case, using "inline" xbl
Comment 10 Allan Beaufour 2005-04-25 01:45:14 PDT
(In reply to comment #8)
> Created an attachment (id=181662) [edit]
> Few fixes, cleaning up things

Why change all the return values from NS_IMETHODIMP to nsresult in
nsXFormsControlStubBase? Aren't they interface implementations?
Comment 11 Olli Pettay [:smaug] (vacation Aug 25-28) 2005-04-25 02:32:14 PDT
(In reply to comment #10)
> (In reply to comment #8)
> > Created an attachment (id=181662) [edit] [edit]
> > Few fixes, cleaning up things
> 
> Why change all the return values from NS_IMETHODIMP to nsresult in
> nsXFormsControlStubBase? Aren't they interface implementations?

nsXFormsControlStubBase does not inherit nsXFormsXMLVisualStub.
So the methods, which return nsresult aren't actually interface
implementations. And some methods do return NS_IMETHODIMP, those which
are from nsIXFormsControl.

Comment 12 Olli Pettay [:smaug] (vacation Aug 25-28) 2005-04-25 14:27:50 PDT
Created attachment 181804 [details] [diff] [review]
Label should work now inside repeats

This patch has a hack to delay the refreshing of the XBL widgets when they
are attached to the elements. Also try-catch is needed in many places in XBL
to ensure that we don't see unnecessary exceptions.
Comment 13 Olli Pettay [:smaug] (vacation Aug 25-28) 2005-04-26 12:25:29 PDT
Created attachment 181889 [details] [diff] [review]
<output> inside <repeat> or <message> works.
Comment 14 Olli Pettay [:smaug] (vacation Aug 25-28) 2005-04-27 12:36:27 PDT
Created attachment 181997 [details] [diff] [review]
XBL widgets initialized properly (without using a timer)

This might be almost the final patch. Now just trying to get the reviews for
Bug 291026
Comment 15 aaronr 2005-05-11 02:07:59 PDT
I applied the patch for 291026 on a build from 05/09 and then this patch on top
of it.  It won't build on Windows due to the call to SetClassAttributeName in
::Created  in nsXFormsControlStub.cpp on the aWrapper.  SetClassAttributeName
isn't a method on a nsIXTFElementWrapper, but rather a
nsIXTFStyledElementWrapper.  Shouldn't there be a QI before the call?  I also
had a problem with nsXFormsOutputElement::Bind calling
nsXFormsControlStub::Bind() directly since it now inherits from
nsXFormsControlStubBase.

Could be I screwed up when fixing up the diff, too. :=)  But could someone
please see if my errors were just my machine before this patch gets checked in?
 Thanks.
Comment 16 Olli Pettay [:smaug] (vacation Aug 25-28) 2005-05-11 02:13:14 PDT
(In reply to comment #15)
> I applied the patch for 291026 on a build from 05/09 and then this patch on top
> of it.  
> ...
> Could be I screwed up when fixing up the diff, too. :=)  But could someone
> please see if my errors were just my machine before this patch gets checked in?
>  Thanks.

Sorry, this patch is not updated to work with the latest patch in bug 291026.
Comment 17 Olli Pettay [:smaug] (vacation Aug 25-28) 2005-05-12 11:48:28 PDT
Created attachment 183423 [details] [diff] [review]
works with the latest patch in bug 291026

testcase:
http://www.cs.helsinki.fi/u/pettay/moztests/xforms/dynamic_ui/calc_svg.xhtml
Comment 18 Olli Pettay [:smaug] (vacation Aug 25-28) 2005-05-25 05:42:36 PDT
Comment on attachment 183423 [details] [diff] [review]
works with the latest patch in bug 291026

Time to review? XTFB got the sr+
Comment 19 aaronr 2005-05-25 18:35:31 PDT
Here is the functionality that we currently have with XTF that we don't have
with this XBL patch (as far as I can tell, running through most of my
testcases).  Should we wait until they are addressed, or fix them all as
seperate bugs after this patch is in?

1) readonly='true()' MIP doesn't get reflected when specified on bindings to
inputs.  Input values can be changed.
2) no submit element
3) clicking/unclicking checkbox doesn't change bound data
4) removing binding attributes via script doesn't work well.
5) output elements contained inside repeat elements don't work (using the repeat
as the context for their value).
6) If I have an ephemeral message that contains a xhtml:input as a child, that
xhtml:input is not focusable.
7) setfocus element doesn't work
8) minimal trigger doesn't fire domactivates when clicked.
9) input with incremental="true" doesn't work.
Comment 20 Allan Beaufour 2005-05-26 00:07:35 PDT
(In reply to comment #19)
> Here is the functionality that we currently have with XTF that we don't have
> with this XBL patch (as far as I can tell, running through most of my
> testcases).  Should we wait until they are addressed, or fix them all as
> seperate bugs after this patch is in?

Thanks for testing it Aaron. I haven't look at the patch yet, but I think we
should try fixing your comments now. If some of them are really troublesome,
then yes, then let's create new bugs. But let's see if we cannot fix most of
them now
Comment 21 Olli Pettay [:smaug] (vacation Aug 25-28) 2005-05-26 00:24:13 PDT
(In reply to comment #20)
> (In reply to comment #19)
> > Here is the functionality that we currently have with XTF that we don't have
> > with this XBL patch (as far as I can tell, running through most of my
> > testcases).  Should we wait until they are addressed, or fix them all as
> > seperate bugs after this patch is in?
> 
> Thanks for testing it Aaron. I haven't look at the patch yet, but I think we
> should try fixing your comments now. If some of them are really troublesome,
> then yes, then let's create new bugs. But let's see if we cannot fix most of
> them now

hmm, 5. was working, I think, apparently I have to re-test.
6. is weird test case ;)

I'll update the patch soon(ish).

thanks for testing Aaron.
Comment 22 aaronr 2005-05-26 14:58:33 PDT
Ok, since it looks like you'll try to fix it before checkin, better point you to
some testcase :=)  Didn't want to clog up the attachment list.  Most live in
bugzilla already, anyhow.

1) readonly input problem -
https://bugzilla.mozilla.org/attachment.cgi?id=173199.  Shouldn't be able to
edit the 'total' field.
2) submit button not implemented -
http://xformstest.org/mozilla/2005/02/03/test-get-1.xhtml.  Should see a
'submit' control following the input controls.
3) checkbox not updating bound data -
https://bugzilla.mozilla.org/attachment.cgi?id=174637  Checking and unchecking
the checkbox should change the value next to the submit control.
4) manipulating binding attrs via DOM -
https://bugzilla.mozilla.org/attachment.cgi?id=178989  Tough to explain in a
couple of sentences.  Use latest nightly next to a XBL patched build, you'll see
the differences in clicking the buttons.
5) repeat with outputs problem -
https://bugzilla.mozilla.org/attachment.cgi?id=168730  Should see the numbers
10, 30 and 45
6) ephemeral containing html:input - will attach testcase
7) set focus problem - I will attach testcase
8) minimal trigger - user error.  Seems to work when I try it now.  However,
won't generate a DOMActivate when clicking with the right mouse button with XBL
patch, but does with XTF.  Which is behavior is right?  testcase:
https://bugzilla.mozilla.org/attachment.cgi?id=175205
9) incremental input - https://bugzilla.mozilla.org/attachment.cgi?id=171769
Comment 23 aaronr 2005-05-26 15:04:12 PDT
Created attachment 184620 [details]
setfocus testcase

testcase for setfocus.	Click on the trigger, input focus should move to the
input control
Comment 24 aaronr 2005-05-26 15:07:33 PDT
Created attachment 184622 [details]
testcase for ephemeral message

testcase for ephemeral message containing a html:input.  Click on the trigger,
you'll see some html appear.  click on the input field with the left mouse. 
Under XTF, this input will get input focus.  Under XBL, message will disappear.
Comment 25 Olli Pettay [:smaug] (vacation Aug 25-28) 2005-06-05 10:30:59 PDT
I've fixed other test cases except 6, which is quite strange one and not
working well with XTF controls either.
In 8 the old behaviour is wrong.

Patch coming.
Comment 26 Olli Pettay [:smaug] (vacation Aug 25-28) 2005-06-05 14:24:51 PDT
Created attachment 185422 [details] [diff] [review]
for review

Allan, maybe you could review this one too ;)
Comment 27 Allan Beaufour 2005-06-06 06:57:58 PDT
(In reply to comment #17)
> Created an attachment (id=183423) [edit]
> works with the latest patch in bug 291026
> 
> testcase:
> http://www.cs.helsinki.fi/u/pettay/moztests/xforms/dynamic_ui/calc_svg.xhtml

Try this WITHOUT this patch, and switch between Normal and SVG twice... my build
crashes in layout/style/nsStyleContext.cpp... can you look at that Olli?
Comment 28 Allan Beaufour 2005-06-06 07:20:22 PDT
Created attachment 185469 [details]
Testcase: Output with invalid binding

This one segfaults Mozilla...
Comment 29 Allan Beaufour 2005-06-06 07:25:27 PDT
Created attachment 185470 [details]
Testcase: Dynamic insertion of output

This does not work with the patch. Feel free to ignore it for now, and create a
new bug though (just remember it :) ).
Comment 30 Olli Pettay [:smaug] (vacation Aug 25-28) 2005-06-06 12:53:27 PDT
Created attachment 185495 [details] [diff] [review]
for review 2

The crash was (again) from the nsXFormsUtils::ReportError.
I changed the serialization so that we don't clone nodes anymore.
I think that has been a problem earlier too.

The dynamic addition of an <output> is not yet fixed :(
Comment 31 Allan Beaufour 2005-06-07 02:31:20 PDT
Created attachment 185546 [details]
Testcase: Listening for xforms-ready

This fails. I listen for the xforms-ready event, which should only be fired
when all controls are initialized. So I would expect to be able to get the
correct value?
Comment 32 Allan Beaufour 2005-06-07 03:02:39 PDT
The output with "Node: 2" is not initialized correctly in attachment 181965 [details].
Comment 33 Allan Beaufour 2005-06-07 03:32:02 PDT
(In reply to comment #27)
> (In reply to comment #17)
> > Created an attachment (id=183423) [edit] [edit]
> > works with the latest patch in bug 291026
> > 
> > testcase:
> > http://www.cs.helsinki.fi/u/pettay/moztests/xforms/dynamic_ui/calc_svg.xhtml
> 
> Try this WITHOUT this patch, and switch between Normal and SVG twice... my build
> crashes in layout/style/nsStyleContext.cpp... can you look at that Olli?

Created bug 296927 for that.
Comment 34 Allan Beaufour 2005-06-07 03:35:01 PDT
Comment on attachment 185495 [details] [diff] [review]
for review 2

Besides comment 32, it looks pretty good.

Create new bugs for the dynamic and xforms-ready issues, and create an up to
patch and I'll do code review.
Comment 35 Olli Pettay [:smaug] (vacation Aug 25-28) 2005-06-09 10:46:17 PDT
Created attachment 185798 [details] [diff] [review]
for review 3

up-to-date and comment #32 should be fixed.
Comment 36 Allan Beaufour 2005-06-13 02:43:30 PDT
Comment on attachment 185798 [details] [diff] [review]
for review 3

> Index: nsXFormsLabelElement.cpp
> ===================================================================

>  /**
>   * Implementation of the XForms \<label\> element.
>   * This constructs an anonymous \<span\> to hold the inline content.
>   */

Fix comment.

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

> +nsPostRefresh::~nsPostRefresh()
> +{
> +#ifdef DEBUG_smaug
> +  printf("~nsPostRefresh\n");
> +#endif
> +  --sRefreshing;
> +  if (sPostRefreshList && !sRefreshing) {
> +    while (sPostRefreshList->Count()) {
> +      PRInt32 last = sPostRefreshList->Count() - 1;
> +      nsIXFormsControl* control =
> +        NS_STATIC_CAST(nsIXFormsControl*, sPostRefreshList->ElementAt(last));
> +      sPostRefreshList->RemoveElementAt(last);
> +      if (control)
> +        control->Refresh();
> +    }

The reason why you keep refreshing the last element is that refresh can lead
to additions/deletions in sPostRefreshList, right? Add a note about it.

> +nsXFormsModelElement::NeedsPostRefresh(nsIXFormsControl* aControl)
> +{
> +  if (sRefreshing) {
> +    if (!sPostRefreshList) {
> +      sPostRefreshList = new nsVoidArray();
> +      if (!sPostRefreshList)
> +        return;

A warning please.

> Index: nsXFormsModelElement.h
> ===================================================================

> +/**
> + * nsPostRefresh is needed by the UI Controls, which are implemented in
> + * XBL and used inside \<repeat\>.
> + */

Yes, but what is the purpose of it?

> Index: nsXFormsOutputElement.cpp
> ===================================================================
>  nsXFormsOutputElement::Bind()
>  {
> -  if (!mValue || !mHasParent)
> -    return NS_OK;
> +  nsXFormsDelegateStub::Bind();
> +
> +  mBoundNode = nsnull;
> 
>    nsresult rv;
>    rv = mElement->HasAttribute(NS_LITERAL_STRING("ref"), &mHasBinding);
>    NS_ENSURE_SUCCESS(rv, rv);
>    if (!mHasBinding) {
>      rv = mElement->HasAttribute(NS_LITERAL_STRING("bind"), &mHasBinding);
>      NS_ENSURE_SUCCESS(rv, rv);
>    }
> 
> +    nsCOMPtr<nsIDOMXPathResult> result;
>    if (mHasBinding) {
> -    rv = nsXFormsControlStub::Bind();
> +    rv = ProcessNodeBinding(NS_LITERAL_STRING("ref"),
> +                            nsIDOMXPathResult::FIRST_ORDERED_NODE_TYPE,
> +                            getter_AddRefs(result));
>    } else {
> -    mBoundNode = nsnull;
> -    // This call does a bit too much, as we do the call again in Refresh(),
> -    // but we need to clear index listeners, bind to the model, get
> -    // evt. context, etc. in Bind().
> -    rv = ResetBoundNode(NS_LITERAL_STRING("value"),
> -                        nsIDOMXPathResult::STRING_TYPE);
> -  }  
> +    rv = ProcessNodeBinding(NS_LITERAL_STRING("value"),
> +                            nsIDOMXPathResult::STRING_TYPE,
> +                            getter_AddRefs(result));
> +  }
> +  
>    NS_ENSURE_SUCCESS(rv, rv);

Why all the changes here? The first ::Bind() does a lot of work that is
canceled by setting mBoundNode = nsnull, and then redone when mHasBinding is
true. What is the problem with the original approach?

> -  return rv;
> +  if (result) {
> +    result->GetSingleNodeValue(getter_AddRefs(mBoundNode));
> +  }
> +
> +  return NS_OK;
>  }

> Index: nsXFormsTriggerElement.cpp
> ===================================================================

> -NS_NewXFormsTriggerElement(nsIXTFElement **aResult)
> -{
> -  *aResult = new nsXFormsTriggerElement();
> -  if (!*aResult)
> -    return NS_ERROR_OUT_OF_MEMORY;
> +  nsCOMPtr<nsIXFormsUIWidget> widget(do_QueryInterface(mElement));
> +  if (widget) {
> +    widget->Disable(aDisable);
> +  }

Do we really need a special function for that. Can we not just set an
attribute as HTML controls do?

> Index: nsXFormsUtils.h
> ===================================================================

> +#define NS_NAMESPACE_MOZ_XFORMS_TYPE     "http://www.mozilla.org/projects/xforms/type"

Should we include a year?

> +++ nsIXFormsDelegate.idl	2005-06-09 17:37:48.000000000 +0300

> + * Portions created by the Initial Developer are Copyright (C) 2004
> + * the Initial Developer. All Rights Reserved.
2005

> +interface nsIXFormsDelegate : nsISupports
> +{
> +  attribute DOMString value;
> +
> +  readonly attribute boolean isReadonly;
> +  readonly attribute boolean isEnabled;
> +  readonly attribute boolean isRequired;
> +  readonly attribute boolean isValid;
> +  readonly attribute boolean hasBoundNode;
> +  void widgetAttached();
> +};

Add documentation for each attribute and the function.

> +++ nsIXFormsUIWidget.idl	2005-06-09 17:37:48.000000000 +0300
> + * Portions created by the Initial Developer are Copyright (C) 2004
> + * the Initial Developer. All Rights Reserved.

2005

> +interface nsIXFormsUIWidget : nsIDOMElement
> +{
> +  void refresh();
> +  boolean focus();
> +  void attributeChanged(in AString name, in boolean removed);
> +  void disable(in boolean disable); // for <submit>
> +};

Again, documentation for each function.

> +++ nsXFormsDelegateStub.h	2005-06-09 17:37:48.000000000 +0300
> + * Portions created by the Initial Developer are Copyright (C) 2004
> + * the Initial Developer. All Rights Reserved.

2005

> +enum nsRepeatState {
> +  eType_Unknown,
> +  eType_Template,
> +  eType_GeneratedContent,
> +  eType_NotApplicable
> +};

Documentation.

And shouldn't this handling be in a class "closer" to nsIXFormsControl
instead? It should be valid for all controls.

> +protected:
> +  nsresult GetState(const nsAString &aState, PRBool *aStateVal);
> +  nsRepeatState UpdateRepeatState();
> +  nsString      mControlType;
> +  nsRepeatState mRepeatState;
> +};

Documentation.

> +++ nsXFormsDelegateStub.cpp	2005-06-09 17:37:48.000000000 +0300
> + * Portions created by the Initial Developer are Copyright (C) 2004
> + * the Initial Developer. All Rights Reserved.

2005

> +nsXFormsDelegateStub::AttributeSet(nsIAtom *aName, const nsAString &aNewValue)
> +{
> +  nsXFormsBindableControlStub::AttributeSet(aName, aNewValue);
> +
> +  nsCOMPtr<nsIXFormsUIWidget> widget(do_QueryInterface(mElement));
> +  if (widget && aName) {
> +    nsAutoString name;
> +    aName->ToString(name);
> +    widget->AttributeChanged(name, PR_FALSE);
> +  }

Why? Can this not be solved with inherited attributes in XBL? If not, include
a comment about it, here or in the interface. Interface I think.

In fact, you are not using it are you?

> +nsXFormsDelegateStub::Refresh()
> +{
> +  if (mRepeatState == eType_Template)
> +    return NS_OK;
> +
> +  NS_NAMED_LITERAL_STRING(mozTypeNs, NS_NAMESPACE_MOZ_XFORMS_TYPE);
> +  NS_NAMED_LITERAL_STRING(mozType, "type");
> +
> +  if (mModel && mBoundNode) {
> +    nsAutoString type, ns;
> +    nsresult rv = mModel->GetTypeAndNSFromNode(mBoundNode, type, ns);
> +    NS_ENSURE_SUCCESS(rv, rv);
> +
> +    ns.AppendLiteral("#");
> +    ns.Append(type);
> +    mElement->SetAttributeNS(mozTypeNs, mozType, ns);
> +  } else {
> +    mElement->RemoveAttributeNS(mozTypeNs, mozType);
> +  }

Extract the type code, so it can be used by nsXFormsOutput too, instead of the
duplicated code.

> +nsXFormsDelegateStub::TryFocus(PRBool* aOK)
> +{
> +  nsresult rv = NS_OK;
> +  if (GetRelevantState()) {
> +    nsCOMPtr<nsIXFormsUIWidget> widget = do_QueryInterface(mElement);
> +    NS_ENSURE_STATE(widget);

Wouldn't it be ok to just return false instead of failing if there's no widget?

> +nsXFormsDelegateStub::GetValue(nsAString& aValue) 
> +{
> +  aValue = EmptyString();

Return null if there's no bound node?

> +nsXFormsDelegateStub::GetHasBoundNode(PRBool *aHasBoundNode)
> +{
> +  *aHasBoundNode = (mBoundNode != nsnull);

*aHasBoundNode = mBoundNode ? PR_TRUE : PR_FALSE;
instead to avoid explicit nsnull check?

> +++ xforms.xml	2005-06-09 17:37:48.000000000 +0300

Include the Mozilla copyright header.
Comment 37 Olli Pettay [:smaug] (vacation Aug 25-28) 2005-06-14 15:10:13 PDT
Created attachment 186257 [details] [diff] [review]
for review 4

I wanted to keep void disable(in boolean disable) in nsIXFormsUIWidget.
Comment 38 Allan Beaufour 2005-06-15 01:09:16 PDT
(In reply to comment #37)
> Created an attachment (id=186257) [edit]
> for review 4
> 
> I wanted to keep void disable(in boolean disable) in nsIXFormsUIWidget.

Why?
Comment 39 Allan Beaufour 2005-06-15 02:32:58 PDT
My version of the calculator that does not use table for layout fails to fresh
the controls when we change the appearance:
http://lab.cph.novell.com/~abeaufour/xforms/calc_svg.xhtml

(this is not new, it was in attachment 185798 [details] [diff] [review] too)
Comment 40 timeless 2005-06-15 09:12:58 PDT
i don't think a warning for when you're running out of memory is particularly
useful. just deal with it.
Comment 41 Olli Pettay [:smaug] (vacation Aug 25-28) 2005-06-20 13:49:48 PDT
Created attachment 186847 [details] [diff] [review]
for review 4 (up-to-date)
Comment 42 Olli Pettay [:smaug] (vacation Aug 25-28) 2005-06-20 14:18:35 PDT
Comment on attachment 186847 [details] [diff] [review]
for review 4 (up-to-date)

I don't remember now who is 
on vacation and when...but
maybe Doron could look at this too.
Comment 43 Allan Beaufour 2005-06-20 23:58:44 PDT
(In reply to comment #42)
> (From update of attachment 186847 [details] [diff] [review] [edit])
> I don't remember now who is 
> on vacation and when...but
> maybe Doron could look at this too.
 
Sorry for the delay... I was in Amsterdam last week, etc. etc. But again, a
textual description of what you did from r3 to r4 would have been nice.
Comment 44 Doron Rosenberg (IBM) 2005-06-21 14:38:00 PDT
Why for example does focus() return a boolean?  It is always returned as true...
Comment 45 Allan Beaufour 2005-06-22 06:12:33 PDT
Comment on attachment 186847 [details] [diff] [review]
for review 4 (up-to-date)

* I will keep saying this: Please include a brief description of what has
  happened between revisions

* Nice with some good comments, including links!

* You seem to have fixed it all, as far as I can see. I haven't had time to
push it through all my tests, but not that much has changed, so I hope it still
works :)

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

>+nsPostRefresh::~nsPostRefresh()
>+{
>+#ifdef DEBUG_smaug
>+  printf("~nsPostRefresh\n");
>+#endif
>+  --sRefreshing;
>+  if (sPostRefreshList && !sRefreshing) {
>+    while (sPostRefreshList->Count()) {
>+      // Iterating from last to first saves possibly few memcopies,
>+      // see nsVoidArray::RemoveElementsAt().

But why do you iterate? You delete the list at the end of the function. [1]

>+      PRInt32 last = sPostRefreshList->Count() - 1;
>+      nsIXFormsControl* control =
>+        NS_STATIC_CAST(nsIXFormsControl*, sPostRefreshList->ElementAt(last));
>+      sPostRefreshList->RemoveElementAt(last);
>+      if (control)
>+        control->Refresh();
>+    }
>+    delete sPostRefreshList;
>+    sPostRefreshList = nsnull;
>+  }
>+}

>Index: nsXFormsDelegateStub.h
>===================================================================

>+enum nsRepeatState {
>+  eType_Unknown,
>+  eType_Template,
>+  eType_GeneratedContent,
>+  eType_NotApplicable
>+};

You didn't answer my question in comment 36:
And shouldn't this handling be in a class "closer" to nsIXFormsControl instead?
It should be valid for all controls.

But it's not terribly important, we can always move it.

r=me, with either a fix or a comment for [1] above.

And once again, sorry for the time it took me to get to this
Comment 46 Olli Pettay [:smaug] (vacation Aug 25-28) 2005-06-23 00:04:16 PDT
I won't have time to watch tinderbox before Sunday night,
but if anyone wants to check this in, please do so.

The comment Allan wants could be
      // Iterating this way because refresh can lead to
      // additions/deletions in sPostRefreshList.
Comment 47 Allan Beaufour 2005-06-23 00:46:34 PDT
(In reply to comment #46)
> I won't have time to watch tinderbox before Sunday night,
> but if anyone wants to check this in, please do so.

I do not have the time either :(

> The comment Allan wants could be
>       // Iterating this way because refresh can lead to
>       // additions/deletions in sPostRefreshList.

Exactly.
Comment 48 Doron Rosenberg (IBM) 2005-06-23 07:39:31 PDT
I have time today, will take care of it
Comment 49 Olli Pettay [:smaug] (vacation Aug 25-28) 2005-06-26 12:46:27 PDT
checked in

Note You need to log in before you can comment on or make changes to this bug.