Closed Bug 1296823 Opened 8 years ago Closed 8 years ago

[GSOC 2016] Event/task UI: refactor iframe load handling and remove use of 'parent'

Categories

(Calendar :: Dialogs, defect)

Lightning 5.3
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: pmorris, Assigned: pmorris)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 5 obsolete files)

In the UI for editing events/tasks, make the parent context's load handler function fire first and the iframe's load handler function fire second, for both tabs and window dialogs.  

The iframe's content will have to load second for windows to allow dynamic assignment of the iframe's src/url as either a XUL or HTML file.  (See patch for bug 1294534.)  So the load order should be the same for tabs for consistency.

This also allows improvements such as eliminating the need to use 'parent' in the iframe context to access info in the parent context.  Then we can only use message passing giving better separation.
Summary: [GSOC 2016] Event/task dialog: refactor iframe load handling and remove use of 'parent' → [GSOC 2016] Event/task UI: refactor iframe load handling and remove use of 'parent'
Attached patch refactor-iframe-load-handling-aug19.patch (obsolete) β€” β€” Splinter Review
This patch makes the iframe's load handler fire second and parent context's fire first.  Removes uses of 'parent' in iframe code.
Assignee: nobody → paul
Attachment #8783149 - Flags: review?(philipp)
Attachment #8783149 - Flags: review?(makemyday)
Attached patch refactor-iframe-load-handling-aug20.patch (obsolete) β€” β€” Splinter Review
This version plays nicely with other pending patches, see comment 3 here:

https://bugzilla.mozilla.org/show_bug.cgi?id=1295392#c3
Attachment #8783149 - Attachment is obsolete: true
Attachment #8783149 - Flags: review?(philipp)
Attachment #8783149 - Flags: review?(makemyday)
Attachment #8783209 - Flags: review?(makemyday)
Comment on attachment 8783209 [details] [diff] [review]
refactor-iframe-load-handling-aug20.patch

Review of attachment 8783209 [details] [diff] [review]:
-----------------------------------------------------------------

This patch still doesn't apply on top of the patch form bug 1294534 as mentioned at https://bugzilla.mozilla.org/show_bug.cgi?id=1295392#c3 - please cross check this.
Attachment #8783209 - Flags: review?(makemyday) → review-
Attached patch refactor-iframe-load-handling-aug21.patch (obsolete) β€” β€” Splinter Review
This version applies successfully.  (Not sure what happened with the previous one, as it applied for me locally before I uploaded it.)
Attachment #8783209 - Attachment is obsolete: true
Attachment #8783314 - Flags: review?(makemyday)
Attached patch refactor-iframe-load-handling-aug22.patch (obsolete) β€” β€” Splinter Review
Given the changes to other patches, this version applies cleanly on top of them, and it passes all mozmill tests.
Attachment #8783314 - Attachment is obsolete: true
Attachment #8783314 - Flags: review?(makemyday)
Attachment #8783668 - Flags: review?(makemyday)
Comment on attachment 8783668 [details] [diff] [review]
refactor-iframe-load-handling-aug22.patch

Review of attachment 8783668 [details] [diff] [review]:
-----------------------------------------------------------------

r=me, with the below nits fixed and provided the mozmill test passes.

::: calendar/lightning/content/lightning-item-iframe.js
@@ +45,5 @@
>  }
> +// The following variables are set by the load handler function of the
> +// parent context, so that they are already set before iframe content load.
> +// var gTimezonesEnabled;
> +// var gShowLink;

to make clear this are not variable you just have commented out, please make this

// The following variables are set by the load handler function of the
// parent context, so that they are already set before iframe content load:
//   - gTimezoneEnabled
//   - gShowLink

::: calendar/lightning/content/lightning-item-panel.js
@@ +221,5 @@
> +
> +        iframe.setAttribute("id", "lightning-item-panel-iframe");
> +        iframe.setAttribute("flex", "1");
> +
> +        let dialog = document.getElementById('calendar-event-dialog');

double quotes.
Attachment #8783668 - Flags: review?(makemyday) → review+
Makes the two changes from the review, and passes mozmill tests.
Attachment #8783668 - Attachment is obsolete: true
Attachment #8783725 - Flags: review+
Working on the next patch I found some old code that needed to be removed/revised in this patch.  Wish I had caught this earlier in the review cycle.  Looks like fallout from rearranging the patches to apply sequentially.  This patch fixes it and passes mozmill tests.
Attachment #8783725 - Attachment is obsolete: true
Attachment #8783802 - Flags: review?(philipp)
Attachment #8783802 - Flags: review?(makemyday)
Comment on attachment 8783802 [details] [diff] [review]
refactor-iframe-load-handling-aug23.patch

looks good, r=me
Attachment #8783802 - Flags: review?(philipp)
Attachment #8783802 - Flags: review?(makemyday)
Attachment #8783802 - Flags: review+
Status: NEW → ASSIGNED
Version: unspecified → Lightning 5.3
https://hg.mozilla.org/comm-central/rev/d3882120592e
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → 5.3
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: