Closed Bug 326766 Opened 19 years ago Closed 19 years ago

excess refresh due widget loading

Categories

(Core Graveyard :: XForms, defect)

defect
Not set
normal

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)

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
Attached patch patch (obsolete) (deleted) — Splinter Review
Attachment #211455 - Flags: review?(allan)
(In reply to comment #1) > Created an attachment (id=211455) [edit] > patch > yes, this looks right. Have to test though.
Assignee: aaronr → surkov
Status: UNCONFIRMED → NEW
Ever confirmed: true
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-
(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?
(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 :)).
(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.
Attached patch patch (obsolete) (deleted) — Splinter Review
Attachment #211455 - Attachment is obsolete: true
Attachment #212191 - Flags: review?(smaug)
Status: NEW → ASSIGNED
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.
(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()).
Attached patch patch (obsolete) (deleted) — Splinter Review
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)
Attachment #212546 - Flags: review?(allan)
(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 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-
Attached patch patch (obsolete) (deleted) — Splinter Review
with smaug's comment
Attachment #212546 - Attachment is obsolete: true
Attachment #212561 - Flags: review?(smaug)
Attachment #212546 - Flags: review?(allan)
(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 ;)
Attachment #212561 - Flags: review?(allan)
(In reply to comment #13) > Created an attachment (id=212561) [edit] > patch Can you not use mBindAttrsCount for this?
(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 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-
Attached patch patch with mBindAttrCount (obsolete) (deleted) — Splinter Review
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)
Attachment #213206 - Flags: review?(smaug)
Attached patch patch (deleted) — Splinter Review
Attachment #213206 - Attachment is obsolete: true
Attachment #213213 - Flags: review?(allan)
Attachment #213206 - Flags: review?(smaug)
Attachment #213206 - Flags: review?(allan)
Attachment #213213 - Flags: review?(smaug)
Attachment #213213 - Flags: review?(smaug) → review+
Attachment #213213 - Flags: review?(allan) → review+
Checked into trunk
Whiteboard: xf-to-branch
Status: ASSIGNED → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
Blocks: 332853
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: