Closed
Bug 1008187
Opened 11 years ago
Closed 10 years ago
[Camera] Update to use <gaia-header>
Categories
(Firefox OS Graveyard :: Gaia::Camera, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
2.0 S5 (4july)
People
(Reporter: wilsonpage, Assigned: wilsonpage)
References
Details
Attachments
(1 file, 2 obsolete files)
No description provided.
Updated•10 years ago
|
Blocks: gaia-header
Assignee | ||
Comment 1•10 years ago
|
||
Attachment #8430768 -
Flags: ui-review?(amlee)
Attachment #8430768 -
Flags: review?(kgrandon)
Assignee | ||
Comment 3•10 years ago
|
||
yor, kgrandon, gmarty: You should check out the changes I made to `ComponentUtils.style()`. Seems to fix my @import woes.
Flags: needinfo?(yor)
Flags: needinfo?(kgrandon)
Flags: needinfo?(gmarty)
(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.
Flags: needinfo?(yor)
Assignee | ||
Updated•10 years ago
|
Attachment #8430768 -
Flags: review?(yor)
Comment on attachment 8430768 [details]
pull-request (master)
Looks good. Assuming Travis passes.
Attachment #8430768 -
Flags: review?(yor) → review+
Hey Kevin, if this looks good to you, can you land this today? Thanks!
Comment 7•10 years ago
|
||
Comment on attachment 8430768 [details]
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!
Attachment #8430768 -
Flags: review?(kgrandon)
Flags: needinfo?(kgrandon) → needinfo?(wilsonpage)
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.
Assignee | ||
Comment 9•10 years ago
|
||
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
Flags: needinfo?(wilsonpage)
Assignee | ||
Comment 10•10 years ago
|
||
Landed on 'master' https://github.com/mozilla-b2g/gaia/commit/392123a9c53fd9a6ccbb1c81908c3176c0647222
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Updated•10 years ago
|
Target Milestone: --- → 2.0 S3 (6june)
Comment 11•10 years ago
|
||
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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment 12•10 years ago
|
||
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•10 years ago
|
||
Sorry, I asked Wilson to land it. Do you think the timeouts are related to this patch?
Comment 14•10 years ago
|
||
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•10 years ago
|
||
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•10 years ago
|
||
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•10 years ago
|
||
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•10 years ago
|
||
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•10 years ago
|
||
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•10 years ago
|
||
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?
Attachment #8432230 -
Flags: review?(wilsonpage)
Attachment #8432230 -
Flags: review?(kgrandon)
Comment 21•10 years ago
|
||
Comment on attachment 8432230 [details]
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.
Attachment #8432230 -
Flags: review?(kgrandon) → review-
Comment 22•10 years ago
|
||
Comment on attachment 8432230 [details]
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?
Attachment #8432230 -
Flags: review- → review?(kgrandon)
Flags: needinfo?(wilsonpage)
Comment 23•10 years ago
|
||
Comment on attachment 8432230 [details]
component_utils.js fix
Looks fine to me.
Attachment #8432230 -
Flags: review?(kgrandon) → review+
Assignee | ||
Comment 24•10 years ago
|
||
Attachment #8432384 -
Flags: review?(yor)
Flags: needinfo?(wilsonpage)
Assignee | ||
Updated•10 years ago
|
Attachment #8430768 -
Attachment is obsolete: true
Attachment #8430768 -
Flags: ui-review?(amlee)
Assignee | ||
Comment 25•10 years ago
|
||
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.
Flags: needinfo?(kgrandon)
Comment 26•10 years ago
|
||
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.
Flags: needinfo?(kgrandon)
Assignee | ||
Comment 27•10 years ago
|
||
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.
Assignee | ||
Updated•10 years ago
|
Attachment #8432230 -
Attachment is obsolete: true
Attachment #8432230 -
Flags: review?(wilsonpage)
Assignee | ||
Updated•10 years ago
|
Flags: needinfo?(yor)
Comment 28•10 years ago
|
||
Nice work, Wilson! Unit tests pass for me locally and the fix works fine in app. I see Travis has some failures though.
Flags: needinfo?(yor)
Attachment #8432384 -
Flags: review?(yor) → review+
Assignee | ||
Comment 29•10 years ago
|
||
Travis is green now, not sure if we're allowed to land this for v2.0 now are we :-/
Flags: needinfo?(yor)
Comment 30•10 years ago
|
||
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.
Flags: needinfo?(yor)
Updated•10 years ago
|
Flags: needinfo?(gmarty)
Updated•10 years ago
|
Target Milestone: 2.0 S3 (6june) → 2.0 S4 (20june)
Updated•10 years ago
|
Target Milestone: 2.0 S4 (20june) → 2.0 S5 (4july)
Assignee | ||
Updated•10 years ago
|
Attachment #8432384 -
Flags: feedback?(kyee)
Assignee | ||
Updated•10 years ago
|
Attachment #8432384 -
Flags: review?(kyee)
Attachment #8432384 -
Flags: review?(dmarcos)
Attachment #8432384 -
Flags: review+
Comment 31•10 years ago
|
||
Comment on attachment 8432384 [details]
pull-request (master)
This looks great to me.
Attachment #8432384 -
Flags: review?(dmarcos) → review+
Assignee | ||
Comment 32•10 years ago
|
||
Landed on 'master' https://github.com/mozilla-b2g/gaia/commit/32a14364f26bf96996319717351fcb1e7fb48605
Status: REOPENED → RESOLVED
Closed: 10 years ago → 10 years ago
Resolution: --- → FIXED
Attachment #8432384 -
Flags: review?(kyee)
Comment 33•10 years ago
|
||
It seems that, this patch raised bug 1079953. Can you please take a look?
Flags: needinfo?(wilsonpage)
Assignee | ||
Comment 34•10 years ago
|
||
This looks like a window management bug to me. I've NI'd someone relevant on the other bug.
Flags: needinfo?(wilsonpage)
Attachment #8432384 -
Flags: feedback?(caseyyee.ca)
You need to log in
before you can comment on or make changes to this bug.
Description
•