977680 - [Messages] Remove position: sticky as it hurts scrolling performance

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::SMS, defect)

Description

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

In bug 945481 position: sticky has been implemented in order to replace the previous JS code that was emulating it. The previous code was doing reflows, and was also lagging behind the scroll gesture as it relies on scroll events.

In our reference workload is it not obvious to see 2 things:
  1. how much position: sticky, or the old implementation cost
  2. how useful it is

Those 2 things comes from the differences between the reference workload and real user data. By dogfooding as my primary device since a long time I have a user data set that is really different from the workload.  while the workload assume there is a lot of sms/mms per day, my real life usage is more that I receive a few sms/mms per day.

As a result there is many headers (4 in the first screen for example, as I receive 1 sms yesterday, 2 two days ago, 1 three days ago, an 1 4 days ago).

The overhead of moving the header to a fixed position is a lot. With the previous implementation it would have mean 4 reflows, with the current it means 4 new layers. This results into less time for painting and much more checkerboarding.

The fixed header is also not very useful, as I can see very easily at which date belongs a message. In the worst case I have to pan a little bit to see the header, and I don't need it very often too as I'm mostly looking for my email thread with someone and rarely for a specific date.

One thing I'm doing a lot is scrolling though, so in order to optimize the scrolling experience I suggest that we completely remove this fixed header thing.

My custom dataset is maybe not representative of all the users, but it likely represent the worst case scenario were there is only a few messages per day, and in this case the cost of the fixed header is very high, while it is not very useful.

On my device with the patches from bug 921858 and without position: sticky I can't see checkerboarding anymore, while it is omnipresent with position: sticky.

Comment 1

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: bug977680.patch

Ayman, the above patch just remove the fixed header. It makes the scrolling perfomance way much better.

Comment 2

[Julien Wajsberg [:julienw]]

Will wait for Ayman's answer before reviewing.

Comment 3

[Florian Bender]

Instead of dropping sticky headers alltogether, maybe limit the headers to a minimum number? E.g. in comment 0's case only show a "last 7 days" + "older" header if the total number of messages > 15, else show no header at all. Each additional header requires its group to contain at least 10 messages.

Sticky headers are only valuable when there's a long list of entries and you may get lost – in this case they're quite essential.

Comment 4

[Kevin Grandon :kgrandon]

Is there any platform investigation we can do to make this faster? Larger groups seem like a nice win as well, Today, Yesterday, This Week, then  maybe in Month Chunks.

Comment 5

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

La(In reply to Kevin Grandon :kgrandon from comment #4)
> Is there any platform investigation we can do to make this faster? 

My understanding of the platform issue is 2 things:
 - there is much more layers to composite on the screen at the same time
 - we are allocating/deallocating them when they come into view / out of view.

> Larger
> groups seem like a nice win as well, Today, Yesterday, This Week, then 
> maybe in Month Chunks.

In my use case at least, Today and Yesterday are usually already visible on my first screen (with the date instead of the name). Also if you didn't receive an sms in the last 2 days then everything will be grouped under Week which can not be handy if you were looking for your last thread.

So I'm not saying this is a bad idea, but it creates some new UX issues. That + the code changes seems risky to me. And as a lazy guy I prefer to take the 100% safe way ;)

Comment 6

[Mason Chang [Inactive] [:mchang]]

Attachment: Layer Tree w/ Position Sticky

Comment 7

[Mason Chang [Inactive] [:mchang]]

Attachment: Layer Tree w/o Position Sticky

Just tried this patch. We still checkerboard, but nowhere near as bad as with position: sticky.

Comment 8

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Mason Chang [:mchang] from comment #7)
> Created attachment 8383785 [details]
> Layer Tree w/o Position Sticky
> 
> Just tried this patch. We still checkerboard, but nowhere near as bad as
> with position: sticky.

Mason I'm running with the patch from bug 921858 as well. It's hard to checkerboard for me with it.

Comment 9

[ayman maat :maat]

Comment on attachment 8383137 [details] [diff] [review]
bug977680.patch

Sorry I cannot approve this patch at the moment. Everytime i apply it i loose the message input field in the new message composer. The message input field is available and functioning before the patch is applied ...but not there after.

Please look into this and ni? me to review again when you have addressed it. Ni? to Vivien

Comment 10

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to ayman maat :maat from comment #9)
> Comment on attachment 8383137 [details] [diff] [review]
> bug977680.patch
> 
> Sorry I cannot approve this patch at the moment. Everytime i apply it i
> loose the message input field in the new message composer. The message input
> field is available and functioning before the patch is applied ...but not
> there after.
> 
> Please look into this and ni? me to review again when you have addressed it.
> Ni? to Vivien

Ayman I can't reproduce, but anyway if there is any issues with this patch I will address them with Julien as he will likely r- me. I wonder if there is a merge on a wrong branch or something like that.

Can you focus on the headers and give your ui-review based on that? Julien which is seating not far from me already told me he can see with me for regressions if any.

Comment 11

[ayman maat :maat]

(In reply to Vivien Nicolas (:vingtetun) (:21) from comment #10)
> (In reply to ayman maat :maat from comment #9)
> > Comment on attachment 8383137 [details] [diff] [review]
> > bug977680.patch
> > 
> > Sorry I cannot approve this patch at the moment. Everytime i apply it i
> > loose the message input field in the new message composer. The message input
> > field is available and functioning before the patch is applied ...but not
> > there after.
> > 
> > Please look into this and ni? me to review again when you have addressed it.
> > Ni? to Vivien
> 
> Ayman I can't reproduce, but anyway if there is any issues with this patch I
> will address them with Julien as he will likely r- me. I wonder if there is
> a merge on a wrong branch or something like that.
> 
> Can you focus on the headers and give your ui-review based on that? Julien
> which is seating not far from me already told me he can see with me for
> regressions if any.

Hey Vivien. Unfortunately I cannot focus on the headers unless i can send messages to generate a list of message in the inbox... and without the message field, i cannot compose messages...therefore cannot send them :)

nevertheless provisionally, is my interpretation is correct based on the understanding of the proposed solution i gained during the conversation i had with Julien on Friday afternoon, I would say that at the moment not sticking the date headers to the underside of the page header is not an acceptable UX solution. 

We cannot second guess the number of messages a user will or will not receive in a given day. The day headers themselves define a clear grouping for sets of messages and if the user scrolls and looses visibility of the header for the grouping they are looking at their cognitive load is increased and therefore their discomfort during use and their risk of error.

But like i say I would like to test before I give a formal review statement. I am going to try a different build on my phone cos this is not the first bug i have tested today where i have had quite different results to that expected by the developer, results which the developer could not themselves reproduce... therefore the issue, i think, could be to do with the build on my phone.

leaving ni? to me, i will get back to you Vivien.

Comment 12

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

One possible options is to replace the time information which is currently displayed in the rows by the date for messages that has not been received today.

It removes the needs for those headers while maximizing the space for messages. It's not negligeable on small screen :)

Comment 13

[Kevin Grandon :kgrandon]

I guess we might want to come up with a solution that works for all apps? What about a static header at the top of each list, and as the user scrolls, the content gets updated. Other headers could disappear under the static header - basically a poor man's position: sticky. I would think a scroll event listener would work ok in this case.

If you'd like me to work on a prototype, I can probably do so. I'm looking at some contacts scrolling anyway and was considering faking position: sticky. Just let me know.

Comment 14

[Julien Wajsberg [:julienw]]

Kevin, I'd like to try using "position: sticky" without of the extra divs too (this would do the same effect that you're describing too), if that's possible to you.

Comment 15

[Kevin Grandon :kgrandon]

(In reply to Julien Wajsberg [:julienw] from comment #14)
> Kevin, I'd like to try using "position: sticky" without of the extra divs
> too (this would do the same effect that you're describing too), if that's
> possible to you.

I could be mistaken as I don't have a patch, but I believe with position sticky, and no grouping divs, headers would slide over the previous header on the page. We would still probably have some scrolling impact of position sticky.

My preference was to use something like position: fixed on a single header, then have a scroll listener update that content somewhere. In this case, I believe that headers would disappear under the static header, instead of over it. If we do this, then we can of course get rid of the grouping divs that were required for position sticky.

Anyway - I suppose ayman should weigh in on the UX of this (I know it's not too easy to understand in text), and we can probably get started on a prototype in parallel.

Comment 16

[Julien Wajsberg [:julienw]]

Yeah, maybe, it's not always clear to me, I'm only asking to try it as it should be quick to do :)

Comment 17

[Kevin Grandon :kgrandon]

(In reply to Julien Wajsberg [:julienw] from comment #16)
> Yeah, maybe, it's not always clear to me, I'm only asking to try it as it
> should be quick to do :)

Ah right - it would be a pretty quick change to make to get UX feedback on :)

Comment 18

[ayman maat :maat]

(In reply to Kevin Grandon :kgrandon from comment #13)
> I guess we might want to come up with a solution that works for all apps?

Hey Kevin. Yes absolutely. We need to have the same behaviour across all apps.

> What about a static header at the top of each list, and as the user scrolls,
> the content gets updated. Other headers could disappear under the static
> header - basically a poor man's position: sticky. I would think a scroll
> event listener would work ok in this case.
> 
> If you'd like me to work on a prototype, I can probably do so. I'm looking
> at some contacts scrolling anyway and was considering faking position:
> sticky. Just let me know.

I think i picture the behaviour you describe; but hey, to remove any ambiguity or misunderstanding would always be welcome :)
(still leaving ni? to me)

Comment 19

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Kevin Grandon :kgrandon from comment #13)
> I guess we might want to come up with a solution that works for all apps?
> What about a static header at the top of each list, and as the user scrolls,
> the content gets updated. Other headers could disappear under the static
> header - basically a poor man's position: sticky. I would think a scroll
> event listener would work ok in this case.

I was more or less thinking about that as a fallback plan. But instead of replacing the content of the fixed header we can probably use a trick with |background: -moz-element(#header-x)| and update that only. That would avoid the extra reflows.

I'm a bit worried about reading the scroll position of the list thought as it could create extra reflows. It would have been so much easier of the scroll event was smart and send us the scroll positions in details :(

Comment 20

[Gabriele Svelto [:gsvelto]]

(In reply to Kevin Grandon :kgrandon from comment #15)
> I could be mistaken as I don't have a patch, but I believe with position
> sticky, and no grouping divs, headers would slide over the previous header
> on the page. We would still probably have some scrolling impact of position
> sticky.

Yeah, that's what I experienced when I removed the extra DIVs. Basically the headers scroll over each other and the resulting effect is definitely less pretty than what we have now.

Comment 21

[Kevin Grandon :kgrandon]

(In reply to Vivien Nicolas (:vingtetun) (:21) from comment #19)
> I'm a bit worried about reading the scroll position of the list thought as
> it could create extra reflows. It would have been so much easier of the
> scroll event was smart and send us the scroll positions in details :(

This would help a *lot* in serveral places. Do you know if we have a bug filed anywhere for this?

Comment 22

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Kevin Grandon :kgrandon from comment #21)
> (In reply to Vivien Nicolas (:vingtetun) (:21) from comment #19)
> > I'm a bit worried about reading the scroll position of the list thought as
> > it could create extra reflows. It would have been so much easier of the
> > scroll event was smart and send us the scroll positions in details :(
> 
> This would help a *lot* in serveral places. Do you know if we have a bug
> filed anywhere for this?

Yep there is bug 655228.

Comment 23

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: scroll.events.delta.patch

Kevin if you are still trying to implement this, I made a small patch to add the scroll offset to the scroll events. I firstly intented to add deltas, but then it means you have to read the scroll position once and keep a local value to stays in sync with the scroll position, so I ended up returning the scroll offsets.

With this patch scroll events has 2 new properties, event.mozDeltaX and event.mozDeltaY that returns the element.scrollLeft and element.scrollTop positions.

If this patch helps you I will clean it up and s/delta/offset/g.

Comment 24

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 8386643 [details] [diff] [review]
scroll.events.delta.patch

Asking f? so I'm sure you will see it :)

Comment 25

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: scroll.events.delta.patch

I forgot to |hg add| the relevant idl/webidl files to the last patch.

I also s/deltaX/offsetLeft/g and s/deltaY/offsetTop/g in this patch. So you should be able to use event.mozOffsetLeft and event.mozOffsetTop.

Comment 26

[Kevin Grandon :kgrandon]

Comment on attachment 8386668 [details] [diff] [review]
scroll.events.delta.patch

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

Vivien this is awesome! This will help immensely with list scrolling, and will be a huge benefit to the work that we're exploring in bug 975680. I am not yet working on this, bug 975680 is taking over my time, but perhaps we can work on something next week?

Comment 27

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Kevin Grandon :kgrandon from comment #26)
> perhaps we
> can work on something next week?

Between 2 foie gras we can work on anything you want :)

Comment 28

[Ben Kelly [:bkelly, not reviewing]]

Very nice!  What is the outlook for getting scroll delta attributes in the platform and standardized?

Comment 29

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Ben Kelly [:bkelly] from comment #28)
> Very nice!  What is the outlook for getting scroll delta attributes in the
> platform and standardized?

Since bug 655228 has been raised after some discussions on the #whatwg mailing list I believe others may be interested to have it. Reading the bug again it seems like smaug would prefer deltaX/deltaY.

I will try to convert back the patch to deltaX/Y, add some tests and propose a patch on bug 655228.

Comment 30

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: scroll.events.delta.patch

Just rebased on master as they were a lot of changes on the day I started the patch.

Comment 31

[Julien Wajsberg [:julienw]]

Comment on attachment 8383137 [details] [diff] [review]
bug977680.patch

Remove the review flag for now until we find that nothing else can help.

We can also rediscuss this with Ayman.

Comment 32

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: bug977680.poc.patch

I made a small lib and use it in the sms app to see. It keeps the sticky header feature without affecting performance in a noticeable way for me, at the price of a killing the 'pretty transition effect' between 2 sticky headers.

I don't think we can do better without killing performance. I need to clean up a little bit the patch though. Asking f? to julien / kevin in the meantime.

The code could have been optimized a little bit to search for headers next to the previous one instead of starting from the start each time but it is an enhancement that should not prevent this to land.

I should add a few tests too.

Comment 33

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: bug977680.poc.patch

I cleaned up a little bit the patch. Still need to add some tests.

Comment 34

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: bug977680.poc.patch

Seems like the tests were red because of a missing require.

For what it worth I didn't use mozRequestAnimationFrame as it seems to not perform exactly the way we want with APZ. While scrolling very slowly it seems like there is no new frame and then the header is updated too late.

Comment 35

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Vivien Nicolas (:vingtetun) (:21) from comment #34)
> For what it worth I didn't use mozRequestAnimationFrame as it seems to not
> perform exactly the way we want with APZ. While scrolling very slowly it
> seems like there is no new frame and then the header is updated too late.

Seems like maybe that's a bug?

Comment 36

[Kartikaya Gupta (email:[e-mail])]

I don't know much about mozRequestAnimationFrame, so I don't know what the expected behaviour here is, sorry. If you can create a standalone test case and describe the actual/expected behaviour maybe I can take a look but still it's probably more in the scope of other people.

Comment 37

[Julien Wajsberg [:julienw]]

Comment on attachment 8389802 [details] [diff] [review]
bug977680.poc.patch

Vivien, I don't have much time, sorry, but can't we reuse most of the old fixed_header ?
=> https://github.com/mozilla-b2g/gaia/blob/v1.2/apps/sms/js/fixed_header.js

In my previous tests, something that sounded especially faster was using nextElementSibling instead of using a getElementsByTagName at start.

Main improvements were:
* we don't need to fetch all the headers if the one we want is among the first ones (happens a lot in the SMS app)
* we're not accessing a live list (which is probably implemented as a linked list ?)

But please profile this to see if this is still the case.

updateHeaderContent is probably what you want to change. Maybe remove the setTimeout too.

Comment 38

[Kevin Grandon :kgrandon]

Comment on attachment 8389802 [details] [diff] [review]
bug977680.poc.patch

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

Looks good to me. I like the simplicity of the small library.

Comment 39

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: bug977680.patch

Comment 40

[Julien Wajsberg [:julienw]]

Comment on attachment 8390072 [details] [diff] [review]
bug977680.patch

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

I didn't do a proper review (especially didn't try on the device) but this looks good.

Some comments below, especially unit tests stuff, and I'd like that you test one specific case manually.

::: apps/gallery/test/unit/sticky_test.js
@@ +37,5 @@
> +      for (var j = 0; j < 10; j++) {
> +        var row = document.createElement('div');
> +        row.style.height = row.style.width = '60px';
> +        row.style.border = row.style.margin = row.style.padding = '0px';
> +        

nit: white space

@@ +46,5 @@
> +    sticky = document.createElement('div');
> +    sticky.style.height = sticky.style.width = '40px';
> +    sticky.style.position = 'absolute';
> +    sticky.style.top = scrollable.offsetTop +
> +                       scrollable.firstElementChild.offsetTop + 'px';

I wonder if you can not put all of this in sticky_test.html instead?

If that does not work as expected just forget it and keep it here for now.

@@ +50,5 @@
> +                       scrollable.firstElementChild.offsetTop + 'px';
> +  });
> +
> +  teardown(function() {
> +  });

nit: you can remove this teardown

@@ +63,5 @@
> +      this.sinon.spy(scrollable, 'addEventListener');
> +
> +      instance = new StickyHeader(scrollable, sticky);
> +
> +      assert.ok(instance);

"new" always returns an object, so this test is useless

@@ +67,5 @@
> +      assert.ok(instance);
> +      sinon.assert.calledWith(scrollable.getElementsByTagName,
> +                              'header');
> +      sinon.assert.calledWith(scrollable.addEventListener,
> +                              'scroll', instance.refresh);

as discussed, it's testing internal implementation and therefore it's bad practice; testing the behavior as you do below is both enough and better  :)

@@ +94,5 @@
> +      });
> +    });
> +  });
> +
> +  */

as discussed, this test is testing internal implementation and you can simply remove it.

@@ +104,5 @@
> +      scrollable.addEventListener('scroll', function onScroll(e) {
> +        scrollable.removeEventListener('scroll', onScroll);
> +
> +        checkBackgroundImage(src);
> +      });

I think this does not actually work because the scroll event is sent asynchronously; therefore your checkBackgroundImage is never really executed, or maybe executed only after mocha has finished its work.

What I used to do is dispatch the scroll event programmatically to make the test synchronous. (we don't really need to check that the browser sends the scroll event, rather we need to check what happens when our app receives the scroll event)

If you really want to keep it asynchronous (which I don't recommend as this is a recipe for intermittents :) ) you can take a callback in scrollAndCheckBackgroundImage, and use a "done" argument in all your tests.

I haven't run the tests myself though so I might be wrong here.

@@ +143,5 @@
> +
> +    test('Scroll 639: background set to the second header', function() {
> +      scrollAndCheckBackgroundImage(639, 'header_0');
> +    });
> +  });

as we discussed, maybe a test that adds a new header and then call refresh, to see whether the header is correctly updated

::: apps/sms/js/thread_list_ui.js
@@ +249,4 @@
>        parent.previousSibling.remove();
>        parent.remove();
>  
> +      this.sticky.refresh();

can you test on a device how it works when you have only one thread and you delete it? This should move to the empty screen. And then send or receive a message again.

@@ +674,4 @@
>      var threadContainer = document.createElement('div');
>      // Create Header DOM Element
>      var headerDOM = document.createElement('header');
> +    headerDOM.id = 'header_' + timestamp;

nit: add a comment to say why it's useful

::: apps/sms/test/unit/thread_list_ui_test.js
@@ +30,4 @@
>  requireApp('sms/test/unit/mock_thread_ui.js');
>  requireApp('sms/test/unit/mock_action_menu.js');
>  require('/shared/test/unit/mocks/mock_performance_testing_helper.js');
> +require('/shared/test/unit/mocks/mock_sticky.js');

you forgot to include this new file

@@ +181,5 @@
>        setup(function() {
>          ThreadListUI.removeThread(3);
>        });
> +      test('calls StickyHeader.refresh', function() {
> +        assert.isTrue(ThreadListUI.sticky.refresh.called);

nit: use sinon.assert.called

@@ +203,5 @@
>        setup(function() {
>          ThreadListUI.removeThread(2);
>        });
> +      test('no StickyHeader.refresh when not removing a header', function() {
> +        assert.isFalse(ThreadListUI.sticky.refresh.called);

nit: use sinon.assert.notCalled
(or maybe we don't need this test at all ? your call)

::: shared/js/sticky.js
@@ +1,1 @@
> +/* exported StickyHeader */

should we call this file sticky_header.js ?

Comment 41

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: bug977680.patch

Thanks for the feedback. This one fixes the feedback comments.

Asking review to kgrandon since Julien is busy right now :)

Comment 42

[Kevin Grandon :kgrandon]

Comment on attachment 8390531 [details] [diff] [review]
bug977680.patch

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

Overall I think Julien did a pretty good job of commenting here :) I tried this on a device and it seems to work fairly well. I took profiles of each approach, but unfortunately the markers are a bit messed up right now due to tiling I believe so it was hard to come back with any conclusive results. Painting and reflow time were about the same for me on my device though.

::: apps/gallery/test/unit/sticky_header_test.js
@@ +19,5 @@
> +  setup(function() {
> +    loadBodyHTML('./sticky_test.html');
> +
> +    scrollable = document.getElementById('scrollable');
> +    scrollable.style.height = scrollable.style.width = '200px';

As Julien said, we already have some styles in sticky_header_test, should these be there as well?

@@ +49,5 @@
> +  teardown(function() {
> +    instance = null;
> +  });
> +
> +  suite('Global', function() {

nit: most of the stuff in this suite is testing internal implementation seems like, and julien brought that up before, so I'm just flagging it. I don't really think it hurts to leave it in though.

Comment 43

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

https://github.com/mozilla-b2g/gaia/commit/6a70c1909a04f25f370502aebc7b6acfdf6e7c72

Comment 44

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

I had to revert this in https://github.com/mozilla-b2g/gaia/commit/2224accc7356aec7c1321d30812596446aa73e30 because it broke gaia-unit tests on TBPL: https://tbpl.mozilla.org/php/getParsedLog.php?id=36112803&tree=B2g-Inbound

Comment 45

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Wes Kocher (:KWierso) from comment #44)
> I had to revert this in
> https://github.com/mozilla-b2g/gaia/commit/
> 2224accc7356aec7c1321d30812596446aa73e30 because it broke gaia-unit tests on
> TBPL:
> https://tbpl.mozilla.org/php/getParsedLog.php?id=36112803&tree=B2g-Inbound

Sigh. This is the third time I it this bug where the global window env is different between TBPL and Travis. TBPL runs tests into different globals, which is nice and so the tests files can not depends on global variables from others files.

Comment 46

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Fixed the test failure and relanded: https://github.com/mozilla-b2g/gaia/commit/d0d334add8777a953b7db6b32de0c833968ecf2b