Closed
Bug 326766
Opened 19 years ago
Closed 19 years ago
excess refresh due widget loading
Categories
(Core Graveyard :: XForms, defect)
Core Graveyard
XForms
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: surkov, Assigned: surkov)
References
Details
(Keywords: fixed1.8.0.4, fixed1.8.1)
Attachments
(1 file, 5 obsolete files)
(deleted),
patch
|
allan
:
review+
smaug
:
review+
|
Details | Diff | Splinter Review |
User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a1) Gecko/20051212 Firefox/1.6a1
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a1) Gecko/20051212 Firefox/1.6a1
When xbl bindign of xforms control is loaded then it calls nsIXFormsDelegate::widgetAttached() and it can pull refresh in a time when model is not ready yet.
Reproducible: Always
Assignee | ||
Comment 1•19 years ago
|
||
Attachment #211455 -
Flags: review?(allan)
Comment 2•19 years ago
|
||
(In reply to comment #1)
> Created an attachment (id=211455) [edit]
> patch
>
yes, this looks right. Have to test though.
Updated•19 years ago
|
Assignee: aaronr → surkov
Updated•19 years ago
|
Status: UNCONFIRMED → NEW
Ever confirmed: true
Comment 3•19 years ago
|
||
Comment on attachment 211455 [details] [diff] [review]
patch
And good that I tested ;)
This breaks the "Insert new listitem" button in
https://bugzilla.mozilla.org/attachment.cgi?id=198785
(I just picked some random testcase)
Attachment #211455 -
Flags: review?(allan) → review-
Assignee | ||
Comment 4•19 years ago
|
||
(In reply to comment #3)
> (From update of attachment 211455 [details] [diff] [review] [edit])
> And good that I tested ;)
> This breaks the "Insert new listitem" button in
> https://bugzilla.mozilla.org/attachment.cgi?id=198785
> (I just picked some random testcase)
>
Actually the patch brokes xforms:label control. Label without bind waits for refresh call for initializing. I guess controls without bind shouldn't waits for refresh call and more imo refresh() should be called never for such controls. btw what do the specs say about it and what is your opinion?
Assignee | ||
Comment 5•19 years ago
|
||
(In reply to comment #4)
> Actually the patch brokes xforms:label control. Label without bind waits for
> refresh call for initializing. I guess controls without bind shouldn't waits
> for refresh call and more imo refresh() should be called never for such
> controls. btw what do the specs say about it and what is your opinion?
>
I think refresh for labels should be called when
1) label has binding attributes or binding attributes was changed (including removing)
2) label has src attribute or src or src attribute was changed (including removing)
In other cases refresh for label shouldn't be called.
Thease conditions should have a sence for all controls (probably excepting 2.).
Though I suppose there will be some problems for select realization and actually refresh() calls for non-binding controls don't disturb :)).
Comment 6•19 years ago
|
||
(In reply to comment #4)
> (In reply to comment #3)
> > (From update of attachment 211455 [details] [diff] [review] [edit] [edit])
> > And good that I tested ;)
> > This breaks the "Insert new listitem" button in
> > https://bugzilla.mozilla.org/attachment.cgi?id=198785
> > (I just picked some random testcase)
> >
>
> Actually the patch brokes xforms:label control. Label without bind waits for
> refresh call for initializing. I guess controls without bind shouldn't waits
> for refresh call and more imo refresh() should be called never for such
> controls. btw what do the specs say about it and what is your opinion?
The spec. only talks about events, and in what sequence the controls/the form should be ready. How maybe times we call our own refresh() method is our problem :)
That said, we should definately minimize the calls, and right now I suspect that we send too many events.
Assignee | ||
Comment 7•19 years ago
|
||
Attachment #211455 -
Attachment is obsolete: true
Attachment #212191 -
Flags: review?(smaug)
Assignee | ||
Updated•19 years ago
|
Status: NEW → ASSIGNED
Comment 8•19 years ago
|
||
Comment on attachment 212191 [details] [diff] [review]
patch
I think I'd prefer checking whether the element is bound to some model.
So something like
if (!nsXFormsUtils::IsDocumentReadyForBind(domDoc) && mModel)
return NS_OK;
Hmm, or is also && mBoundNode needed...
Didn't test, but I hope that should work.
Assignee | ||
Comment 9•19 years ago
|
||
(In reply to comment #8)
> (From update of attachment 212191 [details] [diff] [review] [edit])
> I think I'd prefer checking whether the element is bound to some model.
> So something like
> if (!nsXFormsUtils::IsDocumentReadyForBind(domDoc) && mModel)
> return NS_OK;
>
> Hmm, or is also && mBoundNode needed...
>
> Didn't test, but I hope that should work.
>
I guess it is wrong check because when document is not ready for bind then mModel and mBoundNode is not initialized yet (see nsXFormsControlStubBase::ProcessNodeBinding()).
Assignee | ||
Comment 10•19 years ago
|
||
Since f.x. itemset is inherited from delegate then we should include a check for nodeset attribute.
Attachment #212191 -
Attachment is obsolete: true
Attachment #212546 -
Flags: review?(smaug)
Attachment #212191 -
Flags: review?(smaug)
Assignee | ||
Updated•19 years ago
|
Attachment #212546 -
Flags: review?(allan)
Comment 11•19 years ago
|
||
(In reply to comment #9)
>
> I guess it is wrong check because when document is not ready for bind then
> mModel and mBoundNode is not initialized yet (see
> nsXFormsControlStubBase::ProcessNodeBinding()).
>
Ah, you're right :)
Comment 12•19 years ago
|
||
Comment on attachment 212546 [details] [diff] [review]
patch
>+ if (UpdateRepeatState() == eType_Template)
>+ return NS_OK;
>+
>+ PRBool hasBindAttr = PR_FALSE;
>+ nsAutoString bindAttrName;
>+ (nsXFormsAtoms::bind)->ToString(bindAttrName);
>+ mElement->HasAttribute(bindAttrName, &hasBindAttr);
>+
>+ PRBool hasRefAttr = PR_FALSE;
>+ nsAutoString refAttrName;
>+ (nsXFormsAtoms::ref)->ToString(refAttrName);
>+ mElement->HasAttribute(refAttrName, &hasRefAttr);
>+
>+ PRBool hasNodesetAttr = PR_FALSE;
>+ nsAutoString nodesetAttrName;
>+ (nsXFormsAtoms::nodeset)->ToString(nodesetAttrName);
>+ mElement->HasAttribute(nodesetAttrName, &hasNodesetAttr);
You don't have to check all these attributes. If one
is found, that is enough.
Attachment #212546 -
Flags: review?(smaug) → review-
Assignee | ||
Comment 13•19 years ago
|
||
with smaug's comment
Attachment #212546 -
Attachment is obsolete: true
Attachment #212561 -
Flags: review?(smaug)
Attachment #212546 -
Flags: review?(allan)
Assignee | ||
Comment 14•19 years ago
|
||
(In reply to comment #12)
>
> You don't have to check all these attributes. If one
> is found, that is enough.
>
It's a habbit from js coding ;)
Assignee | ||
Updated•19 years ago
|
Attachment #212561 -
Flags: review?(allan)
Comment 15•19 years ago
|
||
(In reply to comment #13)
> Created an attachment (id=212561) [edit]
> patch
Can you not use mBindAttrsCount for this?
Comment 16•19 years ago
|
||
(In reply to comment #15)
> (In reply to comment #13)
> > Created an attachment (id=212561) [edit]
> > patch
>
> Can you not use mBindAttrsCount for this?
>
Alexander, did you try Allan't suggestion?
That one seems right to me.
Comment 17•19 years ago
|
||
Comment on attachment 212561 [details] [diff] [review]
patch
And even if this approach was selected, I'd prefer reusing those PRBool and nsAutoString variables in this case. And there are some spelling errors in the comment ;)
Attachment #212561 -
Flags: review?(smaug) → review-
Assignee | ||
Comment 18•19 years ago
|
||
I'm sorry for delay but celebrations of 23 february broke my work ;).
Yes, mBindAttrCount works. Though I cannot find where it is defined :).
Attachment #212561 -
Attachment is obsolete: true
Attachment #213206 -
Flags: review?(allan)
Attachment #212561 -
Flags: review?(allan)
Assignee | ||
Updated•19 years ago
|
Attachment #213206 -
Flags: review?(smaug)
Assignee | ||
Comment 19•19 years ago
|
||
Attachment #213206 -
Attachment is obsolete: true
Attachment #213213 -
Flags: review?(allan)
Attachment #213206 -
Flags: review?(smaug)
Attachment #213206 -
Flags: review?(allan)
Assignee | ||
Updated•19 years ago
|
Attachment #213213 -
Flags: review?(smaug)
Updated•19 years ago
|
Attachment #213213 -
Flags: review?(smaug) → review+
Updated•19 years ago
|
Attachment #213213 -
Flags: review?(allan) → review+
Updated•19 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
Updated•19 years ago
|
Keywords: fixed1.8.0.3,
fixed1.8.1
Updated•19 years ago
|
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
•