1295392 - [GSOC 2016] Simplify iframe message passing code for event/task in a tab

Status: RESOLVED FIXED

(Calendar :: Dialogs, defect)

Description

[Paul Morris [:pmorris]]

Refactoring, streamlining, and simplifying the iframe message passing code.

Comment 1

[Paul Morris [:pmorris]]

Attachment: simplify-iframe-messaging-aug15.patch

Moves rotate functions from iframe to the outer parent context, streamlines some of the message passing code, making things simpler overall.

Comment 2

[[:MakeMyDay]]

Comment on attachment 8781334 [details] [diff] [review]
simplify-iframe-messaging-aug15.patch

This patch doesn't apply on top of those of bug 1283623 - there's one conflict with the ones I recently checked and another with the one with the pending review.

This seems to be also a problem at least for the patch from bug 1294534.

Can sort out this and provide an order for all of the remaining needed for landing and accordingly updated patches?

Comment 3

[Paul Morris [:pmorris]]

(In reply to [:MakeMyDay] from comment #2)
> This patch doesn't apply on top of those of bug 1283623 - there's one
> conflict with the ones I recently checked and another with the one with the
> pending review.
> 
> This seems to be also a problem at least for the patch from bug 1294534.

Sorry about that, still figuring out the mercurial bookmarks workflow.

> Can sort out this and provide an order for all of the remaining needed for
> landing and accordingly updated patches?

Ok, I've got this sorted, and will upload new patches.  Here's the order for applying them:

1 "events-tasks-menu-items-aug14.patch" (Bug 1283623)

2 "event-task-html-ui-aug20.patch" (Bug 1294534)

3 "refactor-iframe-load-handling-aug20.patch" (Bug 1296823)

4 "simplify-iframe-messaging-aug20.patch" (Bug 1295392)

I see some ways to take #4 a little further, but that could be done in a separate patch.

Comment 4

[Paul Morris [:pmorris]]

Attachment: simplify-iframe-messaging-aug20.patch

This version plays nicely with other pending patches, see comment 3 here:

https://bugzilla.mozilla.org/show_bug.cgi?id=1295392#c3

Comment 5

[[:MakeMyDay]]

Comment on attachment 8783210 [details] [diff] [review]
simplify-iframe-messaging-aug20.patch

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

Patches 3 and 4 still do not apply on top of the others like it is described in comment 3. Please cross check this again.

Comment 6

[Paul Morris [:pmorris]]

Attachment: simplify-iframe-messaging-aug21.patch

A new version of the patch (number 4) that now applies successfully.
(The previous versions applied for me locally when I tried that before uploading them, not sure what happened.)

Comment 7

[Paul Morris [:pmorris]]

Attachment: simplify-iframe-messaging-aug23.patch

Applies cleanly on current versions of other patches.  Passes mozmill tests.  Includes some new work that was not part of previous version.

Comment 8

[[:MakeMyDay]]

Comment on attachment 8783816 [details] [diff] [review]
simplify-iframe-messaging-aug23.patch

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

Thanks, just some nits. Please cross check the later patches still apply after updating this.

::: calendar/lightning/content/lightning-item-iframe.js
@@ +782,5 @@
>      if (gNewItemUI) {
> +        itemProps.initialPriority = gConfig.priority;
> +        itemProps.supportsPriority = capSupported("priority");
> +
> +        itemProps.initialPrivacy = gConfig.privacy || 'NONE';

double quotes

@@ +784,5 @@
> +        itemProps.supportsPriority = capSupported("priority");
> +
> +        itemProps.initialPrivacy = gConfig.privacy || 'NONE';
> +        // XXX need to update the privacy options depending on calendar support for them
> +        itemProps.supportsPrivacy = capSupported('privacy');

double quotes

@@ +1858,5 @@
> + * values, these are removed from the UI.
> + *
> + * @param {Object} aArg             Container
> + * @param {string} aArg.privacy     (optional) The new privacy value
> + * @param {string} aArg.priority    (optional) The new priority value

short

@@ +1878,4 @@
>      }
> +
> +    // For tasks, do not include showTimeAs
> +    if (aArg.hasOwnProperty("showTimeAs") && !cal.isEvent(window.calendarItem)) {

better use cal.isToDo(window.calendarItem) instead

Comment 9

[Paul Morris [:pmorris]]

Attachment: simplify-iframe-messaging-aug24.patch

Thanks, this fixes the nits, passes mozmill tests, and the other patches still apply on top of this one.

Comment 10

[[:MakeMyDay]]

https://hg.mozilla.org/comm-central/rev/839870624f03