1008187 - [Camera] Update to use <gaia-header>

Status: RESOLVED FIXED

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

Comment 1

[Wilson Page [:wilsonpage]]

Attachment: pull-request (master)

Comment 3

[Wilson Page [:wilsonpage]]

yor, kgrandon, gmarty: You should check out the changes I made to `ComponentUtils.style()`. Seems to fix my @import woes.

Comment 4

[Yan Or]

(In reply to Wilson Page [:wilsonpage] from comment #3)
> yor, kgrandon, gmarty: You should check out the changes I made to
> `ComponentUtils.style()`. Seems to fix my @import woes.

AWESOME!  It's working!  Please, please, please land this fix today.

Comment 5

[Yan Or]

Comment on attachment 8430768 [details] [review]
pull-request (master)

Looks good.  Assuming Travis passes.

Comment 6

[Yan Or]

Hey Kevin, if this looks good to you, can you land this today?  Thanks!

Comment 7

[Kevin Grandon :kgrandon]

Comment on attachment 8430768 [details] [review]
pull-request (master)

Something weird is going here on travis and I'm not sure what. I've restarted the job 3 times now but it keeps timing out. (Some other jobs are, but not this much).

Can you try rebasing against master and force pushing to re-trigger travis?

Thanks!

Comment 8

[Yan Or]

It keeps timing out here:

  ․․․․․Xlib:  extension "RANDR" missing on display ":99.0".

I suggest if the review looks good that we move on and land this.  A lot of other tickets depend on the @import fix.

Thanks.

Comment 9

[Wilson Page [:wilsonpage]]

I seems to have passed on Gaia-Try [1], going to land this to free up Yan. 

[1] https://hg.mozilla.org/integration/gaia-try/rev/312800e37e84

Comment 10

[Wilson Page [:wilsonpage]]

Landed on 'master' https://github.com/mozilla-b2g/gaia/commit/392123a9c53fd9a6ccbb1c81908c3176c0647222

Comment 11

[Ryan VanderMeulen [:RyanVM]]

Revert for Gaia unit test failures.
Master: https://github.com/mozilla-b2g/gaia/commit/97de0a1b593ff97a26bcbac4e006179ec882466b

https://tbpl.mozilla.org/php/getParsedLog.php?id=40746374&tree=B2g-Inbound

Comment 12

[Kevin Grandon :kgrandon]

Come on guys, I told you to wait for a green travis. Sometimes it's ok to not wait. When someone asks you to wait for it, please wait.

Comment 13

[Yan Or]

Sorry, I asked Wilson to land it.  Do you think the timeouts are related to this patch?

Comment 14

[Kevin Grandon :kgrandon]

It's possible, but I don't know off the top of my head. We should run the tests locally first. If there's some reason we can't reproduce we should try a reduced change on travis, or running just the camera tests on travis.

Comment 15

[Yan Or]

Kevin, you are right.  I did a PR with just the component_utils.js changes and it's failing the test as well:

https://travis-ci.org/mozilla-b2g/gaia/builds/26427721

But another PR for an unrelated gaia-subheader change is passing:

https://travis-ci.org/mozilla-b2g/gaia/builds/26428551

Comment 16

[Kevin Grandon :kgrandon]

It could be that changing component utils to use onload is not working. We may need to spin the event loop inside the test using sinon fake timers after the onload. Let me know if you have trouble and ni? me - I can write a small patch for you.

Comment 17

[Kevin Grandon :kgrandon]

Turns out we're actually getting a platform crash in the set visibility call for some reason... 

https://crash-stats.mozilla.com/report/index/09858a26-ab90-4e5c-92f7-aabd02140531

Appears to be something in the test. While normally we could disable some tests if absolutely necessary, I'm scared to do that when we encounter a crash. Let me look at options and see if I can find a workaround.

Comment 18

[Yan Or]

Something is wrong with Travis.  Now the same PR with just component_utils.js is failing the gaia unit tests.  It passed before.

Comment 19

[Kevin Grandon :kgrandon]

I think the attempted component utils fix may be rearing up a nasty platform bug. It may be possible to workaround, or we may have to wait for the @import fix.

Comment 20

[Yan Or]

Attachment: component_utils.js fix

I commented out the 'visibility' calls and got the unit tests to work.  Can we go with this until we have the permanent platform fix?

Comment 21

[Kevin Grandon :kgrandon]

Comment on attachment 8432230 [details] [review]
component_utils.js fix

We should not have dead code, so R- for that for now. Does changing to onload fix the problem that we were having? I'm still not really seeing any problems with setTimeout, but if onload works, that's fine too.

Comment 22

[Yan Or]

Comment on attachment 8432230 [details] [review]
component_utils.js fix

Yes, in some cases, the rendering was still not right with setTimeout().  I've made the changes you suggested.

Wilson, can you remove component_utils.js from your patch?

Comment 23

[Kevin Grandon :kgrandon]

Comment on attachment 8432230 [details] [review]
component_utils.js fix

Looks fine to me.

Comment 24

[Wilson Page [:wilsonpage]]

Attachment: pull-request (master)

Comment 25

[Wilson Page [:wilsonpage]]

Apologies for landing this broken stuff! At the time Travis was in complete meltdown taking >8hrs to run a build. I took a look at the gaia-try output and which seemed to be green. I guess someone needs to clarify with me what gaia-try is and how to read it properly. I'm guessing Kevin knows.

The `style.visibility` stuff was required to prevent 'FOUC' before the component style-sheet had loaded. Where were you seeing the crash happen? Was it in the unit-tests or on in app on device? If the component utils stuff doesn't fix this then I can't land this Camera patch, as you can see a flicker of unstyled header when first opening the preview-gallery.

Sorry again chaps; let's all sync up on IRC when you're online.

Comment 26

[Kevin Grandon :kgrandon]

It was crashing during unit tests, and likely because travis runs in firefox and tbpl runs in b2g desktop. The unfortunate meltdown was happening because people were testing several hours worth of automated tests =/

I'm a bit slammed this week, but might be able to look at other options. We can explore off-screen positioning, or some other style attribute to avoid FOUC perhaps.

Comment 27

[Wilson Page [:wilsonpage]]

yor: Identified and fixed the crashing issue. Firefox seemed to be crashing with a combination of the `new GaiaHeader()` type instantiation and manipulating the element's `style` property on creation. I have pushed an updated patch that re-instates the FOUC fix and fixes the unit-tests.

Comment 28

[Yan Or]

Nice work, Wilson!  Unit tests pass for me locally and the fix works fine in app.

I see Travis has some failures though.

Comment 29

[Wilson Page [:wilsonpage]]

Travis is green now, not sure if we're allowed to land this for v2.0 now are we :-/

Comment 30

[Yan Or]

Can you break the patch into 2 parts, one for Camera and the other for component_utils?  We can then land just the latter and continue porting apps.

Comment 31

[Diego Marcos [:dmarcos]]

Comment on attachment 8432384 [details] [review]
pull-request (master)

This looks great to me.

Comment 32

[Wilson Page [:wilsonpage]]

Landed on 'master' 

https://github.com/mozilla-b2g/gaia/commit/32a14364f26bf96996319717351fcb1e7fb48605

Comment 33

[Safwan Rahman (:safwan)]

It seems that, this patch raised bug 1079953. 
Can you please take a look?

Comment 34

[Wilson Page [:wilsonpage]]

This looks like a window management bug to me. I've NI'd someone relevant on the other bug.