502936 - cached calendars should not be refreshed at launch/startup time

Status: RESOLVED FIXED

(Calendar :: Internal Components, defect)

Description

[Wolfgang Sourdeau]

User-Agent:       Mozilla/5.0 (X11; U; Linux x86_64; fr; rv:1.9.0.11) Gecko/2009061317 Iceweasel/3.0.11 (Debian-3.0.11-1)
Build Identifier: 20090707041124

When calendars are cached, a getItems is performed on them when the views become available and again when the calendar triggers the "onLoad" notification. The attached patch prevent getItems() operations on such calendars as long as the "onLoad" has not been posted. This enhances responsitivity and load time when a couple of cached calendars are configured.

Reproducible: Always

Comment 1

[Wolfgang Sourdeau]

Attachment: the fix

For a single calendar, this patch reduces the amount of calls to "getItems" on the storage instance goes from 29 down to 8.

Comment 2

[Stefan Sitter [:ssitter]]

Related to bug 412914?

Comment 3

[Daniel Boelzle [:dbo]]

Comment on attachment 387291 [details] [diff] [review]
the fix

This looks like a hack. You should postpone/queue getItems calls until the calendar is loaded (like ics is doing); the caching facade might be the right place.

Comment 4

[Wolfgang Sourdeau]

(In reply to comment #2)
> Related to bug 412914?

Probably one of the causes yes. This bug and #500718 are both related actually.

Comment 5

[Wolfgang Sourdeau]

(In reply to comment #3)
> (From update of attachment 387291 [details] [diff] [review])
> This looks like a hack. You should postpone/queue getItems calls until the
> calendar is loaded (like ics is doing); the caching facade might be the right
> place.

The problem is that getItems on storage calendars is a long operation. Postponing calls to getItems will not help because we want to reduce them.
As far as I understand, the "onLoad" event is meant to announce the availability of a calendar, or the availability of new data. Therefore, I think no getItems should occur at all on a calendar as long as the "onLoad" was not triggered for that calendar.

Comment 6

[Wolfgang Sourdeau]

> The problem is that getItems on storage calendars is a long operation.
> Postponing calls to getItems will not help because we want to reduce them.
> As far as I understand, the "onLoad" event is meant to announce the
> availability of a calendar, or the availability of new data. Therefore, I think
> no getItems should occur at all on a calendar as long as the "onLoad" was not
> triggered for that calendar.

Maybe a new "onCalendarReady" notification should be created and taken into account for that case?

Comment 7

[Wolfgang Sourdeau]

Attachment: enhanced version

This new version fixes the case where an onOperationComplete would not be triggered when no enabled calendar had emitted an initial "onLoad".

Comment 8

[Daniel Boelzle [:dbo]]

(In reply to comment #5)
> The problem is that getItems on storage calendars is a long operation.
> Postponing calls to getItems will not help because we want to reduce them.
> As far as I understand, the "onLoad" event is meant to announce the
> availability of a calendar, or the availability of new data. Therefore, I think
> no getItems should occur at all on a calendar as long as the "onLoad" was not
> triggered for that calendar.

Right, but you need to keep the API. If a calendar is enabled, it needs to respond to its getItems calls when data has loaded. I think it's wrong to break this API.
I think the core problem is that the views use the composite facade/multiplexer, acting as a single calendar, thus the views will retrieve the same data multiple times when onload events occur. I think the views need to deal with the individual calendars to optimize that. calCompositeCalendar needs to adhere calICalendar..

Comment 9

[Daniel Boelzle [:dbo]]

(In reply to comment #6)
> Maybe a new "onCalendarReady" notification should be created and taken into
> account for that case?
Where's the difference to the current "onload" hint/event?

Comment 10

[Wolfgang Sourdeau]

(In reply to comment #8)

> Right, but you need to keep the API. If a calendar is enabled, it needs to
> respond to its getItems calls when data has loaded. I think it's wrong to break
> this API.

We probably should prevent getItems from the views themselves then... That's where it's a bit tricky.

> I think the core problem is that the views use the composite
> facade/multiplexer, acting as a single calendar, thus the views will retrieve
> the same data multiple times when onload events occur. I think the views need
> to deal with the individual calendars to optimize that. calCompositeCalendar
> needs to adhere calICalendar..

What about another approach... if you move any refresh/reload handling from the views and transfer them exclusively to the calCompositeCalendar, the latter can then become a controller for the different views. I think the paradigm would be consistent and help with this problem as well as the problem where an onLoad triggers the refresh on ALL calendars, no matter what calendar the onLoad originated from. It would also offer an other approach to #500718. What do you think?

Comment 11

[Wolfgang Sourdeau]

(In reply to comment #9)
> (In reply to comment #6)
> > Maybe a new "onCalendarReady" notification should be created and taken into
> > account for that case?
> Where's the difference to the current "onload" hint/event?

Well, it would separate the concept of calendar initialization (some would trigger it from their constructor while others from a callback) from the concept of data refresh (aka "changelog").

Comment 12

[Wolfgang Sourdeau]

Any further comment about this? We are ready to put some effort into improving all of this but we need your opinion so that we know if we are on the right track... (AND to know when we can start working on this)

Comment 13

[Daniel Boelzle [:dbo]]

(In reply to comment #10)
> (In reply to comment #8)
...
> > I think the core problem is that the views use the composite
> > facade/multiplexer, acting as a single calendar, thus the views will retrieve
> > the same data multiple times when onload events occur. I think the views need
> > to deal with the individual calendars to optimize that. calCompositeCalendar
> > needs to adhere calICalendar..
> 
> What about another approach... if you move any refresh/reload handling from the
> views and transfer them exclusively to the calCompositeCalendar, the latter can
> then become a controller for the different views. I think the paradigm would be
> consistent and help with this problem as well as the problem where an onLoad
> triggers the refresh on ALL calendars, no matter what calendar the onLoad
> originated from. It would also offer an other approach to #500718. What do you
> think?

Could you please elaborate a bit more on this? Why does it help? Does such a controller need to implement calICalendar? Is is sensible to stick to that interface at all for the purpose of loading the views? Thanks!

Comment 14

[Wolfgang Sourdeau]

I think all of the above can be ignored or postponed.

I have found a way to achieve the same result without modifying anything in the architecture, by removing all the calls to refresh() from the onLoad and the onEndBatch callbacks. I am going to send another patch a bit later. The gain in display performance is simply astonishing.

Comment 15

[Wolfgang Sourdeau]

Attachment: refresh killer

This is my patch for Lightning 0.9 (Inverse edition). It's only here as a request for comment. Once everyone agrees, I'll adapt it to Lightning 1.0.

Meanwhile, I think this bugreport could be closed. Also, this patch follows the same mindset as 500718. Ultimately, I think it obsoletes the calCompositeCalendar altogether, at least as a calendar-proxy.

Please review it and submit your comments. I necessary, I'll close or rename this bugreport and open a new one a bit more representative of what was solved here.

Comment 16

[Ludovic Marcotte]

Any update on the review for this patch?

It looks to me it should be a rather high-priority item. I've tested yesterday TB3b3 together with a Lightning nightly build with sogo-demo.inverse.ca (sogo1 account) and it was just totally unusable due to the major slowness of the refresh code.

I've then tested with our patch on our Inverse Edition and it was just _fast_.

We just need your input in order to move forward with a real patch for 1.0. I think it should also be a release blocker.

Comment 17

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

I'm fine with blocking for 1.0 (final at latest) since this patch has a driver. I can't say anything about the review right now, Daniel will surely comment soon.

Comment 18

[Wolfgang Sourdeau]

3 weeks and still no updates?

Comment 19

[Daniel Boelzle [:dbo]]

Wolfgang, since you seem to have deeply dived into the batch handling code (and your patch resolves some of it), could you think about an alternative to it? We are encountering problems w.r.t. threading, see bug 494783 comment #26.

Comment 20

[Wolfgang Sourdeau]

I don't deeply know the issue at hand here but I have two ideas:
- is batch mode still useful (after my patch)? It not, drop it.
- if we need to keep it, wouldn't there be a way for the new parsing code to invoke the callbacks from the calling thread?

Besides, do you have any comment one my code at all?

Comment 21

[Wolfgang Sourdeau]

(In reply to comment #20)
> I don't deeply know the issue at hand here but I have two ideas:
> - is batch mode still useful (after my patch)? It not, drop it.

My guess is that batch mode is still useful but only on the backend side, not the views. The rest can work properly with a getItems at initialization and onAdd/onModify/onDelete.

Comment 22

[Simon Paquet [:sipaq]]

Daniel, any update on the review here?

Comment 23

[Daniel Boelzle [:dbo]]

Comment on attachment 387459 [details] [diff] [review]
enhanced version

r- for the mentioned reasons. Moreover, mixing caching conditionals into composite seems to be the wrong approach to me.

Comment 24

[Daniel Boelzle [:dbo]]

Comment on attachment 389767 [details] [diff] [review]
refresh killer

Wolfgang, could you please add a clean patch against comm-central?
In general I am fine with removing batch handling from agenda or unifinder code if it improves performance. It certainly simplifies the code.

Comment 25

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Any updates on this bug?

Comment 26

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Unless there are minimally invasive changes we can make here, I think this bug should be postponed to after 1.0. Given our test coverage is far from optimal, changing such a fundamental part of the calendar interfaces sounds very risky to me.

Wolfgang, Ludovic, is that ok with you?

Comment 27

[Ludovic Marcotte]

Our GSoC student is currently working on this. He's porting the patch to 1.0b2 (Inverse Edition) and then, he'll port it to trunk.

Is 1.0b5 being scheduled here or the next version is 1.0?

In any cases, this patch MUST go in the next version.

Comment 28

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

1.0b5 is a maintenance release for 1.0b4 to fix some urgent issues, and is due towards the weekend. The real "next release" is hopefully 1.0 and given this is part of the soc project I'm happy to block 1.0 for this.

Comment 29

[Ludovic Marcotte]

Ok, looks good to me then.

Comment 30

[Mohit Kanwal [:redDragon]]

Hi everyone, 

Its my pleasure to work on this. 

My summer is turning out to be more fruitful than I thought. =)

Comment 31

[Mohit Kanwal [:redDragon]]

Attachment: Patch ported to Lightning

Patch ported to trunk. And tested.
Looks good so far.

Comment 32

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Comment on attachment 550731 [details] [diff] [review]
Patch ported to Lightning

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

I've given this bug a test and unfortunately hit some problems. Some of it may just require explaining to me why its happening, but I think the reload remote calendar issue needs to be fixed. I'm going to have to deny review for this version of the patch.

::: calendar/base/content/agenda-listbox.js
@@ -918,4 @@
>  };
>  
>  agendaListbox.calendarObserver.onLoad = function() {
> -    this.agendaListbox.refreshCalendarQuery();

Same as in the month view: removing this breaks reload remote calendars

@@ -925,5 @@
>  function observer_onAddItem(item)
>  {
> -  if (this.mBatchCount) {
> -      return;
> -  }

Could you explain why removing batched mode improves things? What it was supposed to do is make sure that multiple changes do not hammer the UI, but I can see that doing a full refresh afterwards isn't right unless maybe there was a substantial amount of changes.

Anyway, I see two options here:

a) Get rid of batch mode altogether (this also means the whole onStartBatch/onEndBatch observer notification) because you think its drawbacks outweigh its merits.

b) Fix batch mode to do what it really should (i.e provide a mechanism for consumers of the batch mode to query up the changes and then do them once when batch mode ends.

::: calendar/base/content/calendar-base-view.xml
@@ +138,2 @@
>              },
>  

Removing this causes the following error:

1. Create an event (i.e on an ICS file:// calendar)
2. Remove the event externally (i.e with a text editor)
3. Reload remote calendars

Expected:
* The event should go away

Actual:
* The event stays around

@@ +229,4 @@
>                      case "readOnly":
>                      case "disabled":
>                          // XXXvv we can be smarter about how we handle this stuff
> +                        //this.calView.refresh();

I fear its not quite that easy. This breaks the following:

1. Create some events in your view
2. Mark the calendar disabled

Expected:
* The events should go away

Actual:
* The events stay around.


It does seem to work for readOnly though, at least from simple testing.

::: calendar/base/content/calendar-month-view.xml
@@ -1053,5 @@
>              for each (let itemData in boxItemData) {
>                let item = itemData.item;
> -              if (!deleteHash[item.hashId] && item.calendar.id == aCalendar.id) {
> -                deleteHash[item.hashId] = true;
> -                this.doDeleteItem(item);

Go ahead and do the same for calendar-multiday-view, the deleteHash variable is not needed.

@@ +1076,4 @@
>            let boxes = this.findDayBoxesForItem(aAlarmItem);
>            for each (var box in boxes) {
>              for each (var itemData in box.mItemData) {
> +              if (itemData.item.hasSameIds(aAlarmItem) && itemData.box) {

This change is no longer needed since bug 424808.

::: calendar/base/content/calendar-unifinder.js
@@ -126,5 @@
>              unifinderTreeView.clearItems();
>          }
> -        if (!this.mInBatch) {
> -            refreshEventTree();
> -        }

Same as in the month view: removing this code breaks the reload remote calendars functionality.

Comment 33

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Attachment: De-bitrotted patch

For your convenience I've de-bitrotted the patch so it applies to the latest comm-central default branch.

Comment 34

[Wolfgang Sourdeau]

> >  
> >  agendaListbox.calendarObserver.onLoad = function() {
> > -    this.agendaListbox.refreshCalendarQuery();
> 
> Same as in the month view: removing this breaks reload remote calendars

I will let Mohit answer that one as he is assumed to have understood what he was working for...

> 
> @@ -925,5 @@
> >  function observer_onAddItem(item)
> >  {
> > -  if (this.mBatchCount) {
> > -      return;
> > -  }
> 
> Could you explain why removing batched mode improves things? What it was
> supposed to do is make sure that multiple changes do not hammer the UI, but
> I can see that doing a full refresh afterwards isn't right unless maybe
> there was a substantial amount of changes.

I can more or less answer that one. Batch mode seems to have been implemented in the views in order to avoid progressive invocations of the onAddItem/onModifyItme and onDeletedItem callbacks, by preventing them from doing anything. This, we want to get rid of. The other aspect of batch mode is to enable transactional updates of the SQLite databases, which generally improves performances, in particular on Windows. That aspect we want to keep.

Why we want to get rid of it in the views is for the following reason: we want to get rid of the call to getItems in the "onLoad" method because that's the original culprit, in that it is invoked each time any calendar is done refreshing, whether any change occurred and because it forces a complete redraw of the current view when done. Instead we want to make use (again?) of the onAddItem/onDeleteItem and onModifyItems methods because they are invoked only when something actually happens in the backend, only by item, by calendar. This way, we only draw or redraw the modified events and only once per event.

Therefore....

> a) Get rid of batch mode altogether (this also means the whole
> onStartBatch/onEndBatch observer notification) because you think its
> drawbacks outweigh its merits.

Yes, we should, unless there was any other reason for the batch handling in the views (see comment #20 above), but only in the views.

> b) Fix batch mode to do what it really should (i.e provide a mechanism for
> consumers of the batch mode to query up the changes and then do them once
> when batch mode ends.

We only have that, as the onAdd/MOdify/DeleteItem are invoked only when needed. So there is no real use for this.

For the rest, I will leave the fixes to Mohit, as, again, he is supposed to have understood what he did and why he did it...

Comment 35

[Wolfgang Sourdeau]

> We only have that, as the onAdd/MOdify/DeleteItem are invoked only when
> needed. So there is no real use for this.

Sorry, typo -> "We only" = "We already"

Comment 36

[Mohit Kanwal [:redDragon]]

Hi Philipp and Wolfgang, 

Finally I have the time to reply to you 

(In reply to Philipp Kewisch [:Fallen] from comment #32)
> ::: calendar/base/content/agenda-listbox.js
> @@ -918,4 @@
> >  };
> >  
> >  agendaListbox.calendarObserver.onLoad = function() {
> > -    this.agendaListbox.refreshCalendarQuery();
> 
I removed this because essentially it was becoming redundant with the batch mode with onEndBatch being fired after a onLoad notification. Hence the views were getting refreshed multiple times. And it was not causing an issue with calendar reload or loss of information wrt events. 

I went inside batch mode and tried to remove all batch mode invocations so that the views don't call it, however, there were still parts for example the tasks view pane in the today-pane that gets refreshed on a batch mode invocation, so I had to keep it. If there is a solution to that issue then we can completely do away with the batch mode and its notifications. 

The previous patch(proposed by Wolfgang) had the same changes, I mirrored them to fit the new code which has undergone a lot of changes after Lightning 0.9

Let me know how should I proceed with this.

Cheers
Mohit

Comment 37

[Mohit Kanwal [:redDragon]]

Attachment: version 3 of the patch with reload remote calendar fixed

This patch fixes the reload remote calendar functionality on disabling a calendar. And removed delete hash from multiday-view.

Comment 38

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Comment on attachment 552051 [details] [diff] [review]
version 3 of the patch with reload remote calendar fixed

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

I'm going to have to set r- here, there are again a few issues, as described below.

::: calendar/base/content/agenda-listbox.js
@@ -922,1 @@
>  };

Still having trouble here. STR:

1. Create an event on an ics calendar that happens tomorrow
2. See it in the today pane
3. Externally change the title of the event
4. Reload remote calendars

Expected:
* Title should be updated 

Actual:
* Title is not changed, neither in the view nor in the today pane.

::: calendar/base/content/calendar-base-view.xml
@@ -141,1 @@
>              },

This is probably the culprit for the views not reloading as described above.

::: calendar/base/content/calendar-task-tree.xml
@@ -844,5 @@
>  
>            onLoad: function tTO_onLoad() {
> -              if (this.mBatchCount == 0) {
> -                  this.binding.refresh();
> -              }

I would have expected this to fail similar to the views above, but surprisingly reloading tasks works! :-)

::: calendar/base/content/calendar-unifinder.js
@@ -131,5 @@
>      },
>  
>      onAddItem: function uO_onAddItem(aItem) {
>          if (isEvent(aItem) &&
> -            !this.mInBatch &&

Since you are no longer using mInBatch, I'd appreciate if you could remove it from the observer alltogether. Similar in other places, if you no longer use a variable then please remove it.

Comment 39

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Comment on attachment 552051 [details] [diff] [review]
version 3 of the patch with reload remote calendar fixed

Thank you splinter reviews for mushing it all up!


> agendaListbox.calendarObserver.onLoad = function() {
>-    this.agendaListbox.refreshCalendarQuery();
> };
These are the lines of the first issue I meant.

>diff -r c63fefad39c0 calendar/base/content/calendar-base-view.xml
>--- a/calendar/base/content/calendar-base-view.xml	Sun Aug 07 02:22:19 2011 +0800
>+++ b/calendar/base/content/calendar-base-view.xml	Wed Aug 10 20:24:26 2011 +0800
> 
>             onLoad: function onLoad() {
>-                this.calView.refresh();
>             },
And these are the second lines, the culprit in the views.

Comment 40

[Mohit Kanwal [:redDragon]]

Attachment: Patch-Version4

Hi all,

In this version, i tried to address the above concern and also removed batch variables from calendar-task-tree. I have done testing on caldav calendars and it works so far.

For ICS, I am not sure how the behavior will be since the icsParser worker thread keeps crashing under the newer comm-central repo. Can someone test it out on ICS provider ?

Thanks
Mohit

Comment 41

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Comment on attachment 554147 [details] [diff] [review]
Patch-Version4

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

Hey Mohit, are you sure you uploaded the right version? I see a few batch variables left in the files, see below. Don't worry about the ics issues, I'll try to fix those soon. If you need to test this in the meanwhile you'll have to build comm-aurora, but that might break other things so don't worry about it.

Since you're almost there I'm setting review+, but please upload a new patch with the batch variables removed (also look out for any definitions of the variables, make sure grep -r mBatchCount calendar/* doesn't return any relevant results)

::: calendar/base/content/agenda-listbox.js
@@ +911,3 @@
>  agendaListbox.calendarObserver.onEndBatch =
>  function() {
>      this.mBatchCount--;

Is this variable used anywhere else? Maybe you can remove it too?

::: calendar/base/content/calendar-base-view.xml
@@ +131,3 @@
>              },
>              onEndBatch: function onEndBatch() {
>                  this.calView.mBatchCount--;

Same here

::: calendar/base/content/calendar-task-tree.xml
@@ +837,3 @@
>  
>            onEndBatch: function tTO_onEndBatch() {
>                this.mBatchCount--;

Same here. Please also check any other batch variables you are touching.

Comment 42

[Mohit Kanwal [:redDragon]]

Hi Philipp, there are some functions like refreshCalendarQuery in agendaListbox.js which make use of the batch variables before doing a refresh query. For example :

agendaListbox.refreshCalendarQuery =
function refreshCalendarQuery(aStart, aEnd, aCalendar) {
    if (this.mBatchCount > 0) {
        return;
    }

Should I remove these statements as well? It sort of serves as an additional guard against refresh during batch operations from the views.

Comment 43

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

(In reply to Mohit Kanwal [:redDragon] from comment #42)

> Should I remove these statements as well? It sort of serves as an additional
> guard against refresh during batch operations from the views.

I'd be ok with that, but please do test for negative effects when you do.

Comment 44

[Mohit Kanwal [:redDragon]]

Attachment: Version[5]

Cleaned up the patch with removal of mBatchCount variables. The only place they exist now is calProviderUtils.jsm which is local to the module and is responsible for firing notifications "onEndBatch" and "onStartBatch". I don't want to remove mBatchCount from calProviderUtils since maybe later people might want to do some stuff based on the notifications. 

Apart from that there are no negative effects and the speed gain after applying this patch is simply astonishing. One can clearly see before applying the patch that the views refresh twice on a "reload remote calendar" command, but they only refresh once after applying the patch.

Comment 45

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Comment on attachment 556762 [details] [diff] [review]
Version[5]

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

r=philipp, but I'd appreciate if you could answer these two questions before I push the patch:

::: calendar/base/content/calendar-unifinder.js
@@ +113,1 @@
>          refreshEventTree();

Is refreshEventTree() needed here?

::: calendar/base/content/widgets/minimonth.xml
@@ +642,5 @@
>        <method name="onLoad">
>          <parameter name="aCalendar"/>
>          <body><![CDATA[
> +            this.resetAttributesForDate();
> +            this.getItems();

Could you explain this change? Not saying its wrong, but I wonder why this hasn't been there before?

Comment 46

[Mohit Kanwal [:redDragon]]

(In reply to Philipp Kewisch [:Fallen] from comment #45)
Hi Philipp,

> ::: calendar/base/content/calendar-unifinder.js
> @@ +113,1 @@
> >          refreshEventTree();
> 
> Is refreshEventTree() needed here?

Yes refreshEventTree() is needed here, otherwise, whenever a "onAddItem" notification occurs, two copies of the same event get added to the unifinder.

> 
> ::: calendar/base/content/widgets/minimonth.xml
> @@ +642,5 @@
> >        <method name="onLoad">
> >          <parameter name="aCalendar"/>
> >          <body><![CDATA[
> > +            this.resetAttributesForDate();
> > +            this.getItems();
> 
> Could you explain this change? Not saying its wrong, but I wonder why this
> hasn't been there before?

Yes I don't know how this slipped through, sorry about that, I think must have happened when I was reconciling Inverse's patch for refresh-killer.

I am uploading a new patch with the lines removed.

Comment 47

[Mohit Kanwal [:redDragon]]

Attachment: [Patch Version-6]

Removed the lines in the onLoad handler of minimonth.xml

Comment 48

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Pushed to comm-central <http://hg.mozilla.org/comm-central/rev/1c7e6fcdc4fa>
-> FIXED

Comment 49

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Backported to comm-aurora <http://hg.mozilla.org/releases/comm-aurora/rev/f49d83800dff>

Comment 50

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Backported to comm-beta <http://hg.mozilla.org/releases/comm-beta/rev/347508f378fd>