mozilla :: #activity-stream

17 Jul 2017
09:28as-github-botursula: Hey! Someone just assigned you a PR for review: https://github.com/mozilla/activity-stream/pull/2867
14:25as-github-botMardak: Hey! Someone just assigned you a PR for review: https://github.com/mozilla/activity-stream/pull/2864
14:32as-github-botpiatra: Hey! Someone just assigned you a PR for review: https://github.com/mozilla/activity-stream/pull/2867
14:35ursulaMardak: ping
14:35Mardakpong
14:36ursulahey, i'm looking at your patch to increase waitForCondition time, can you just explain how removing the 100 and the 30 helps? it's not super clear... does it fall back to something if it doesn't have those params?
14:36ursulai read your comments about how it's just a little too late when activity stream is enabled
14:37ursulaso it makes sense to increase the waitForCondition ... i just don't know how the fallback for that works
14:38Mardakursula: https://searchfox.org/mozilla-central/search?q=waitForCondition&path=browsertestutils.jsm
14:38Mardakdefault parameters
14:39ursulaamazing
14:39ursulawas there a specific reason it was 30 in the first place?
14:40ursulaor was that just some arbitrary number
14:40MardakFischer wrote the code and didn't explicitly say why. probably copy/pasted like the rest of them in head.js
14:41fischerhi
14:42Mardakoh hey!
14:42fischerthe reason using waitForCondition is that
14:42fischeron window load, we call requestIdleCallback to load onboarding
14:44Mardakfischer: yup. but why explicitly retry 30 times instead of the default 50?
14:44dmosecan i propose that we capture the outcome of this conversation as a comment in the code?
14:45dmoseso that if things slow down for some reason in the future, whoever has to debug won't have to re-derive this?
14:45ursulayeah good idea dmose
14:46fischerok i will leave the comment on the bug so clearer
14:46ursulathanks fischer!
14:46dmoseyeah, thanks
14:46Mardakthe code has a comment block "The onboarding overlay is init inside window.requestIdleCallback, not immediately,"
14:46dmoseand thanks Mardak for really tearing it up!
14:47dmoseMardak: maybe something more explicit about simply extending the timeout if it starts getting intermittent?
14:49k88hudsonwhoa! I see only one test failure in the pref on list
14:51ursulaMardak: i'll wait for fischer's comment on why 30 was used and then i'll r+ ? unless dmose wants to do it?
14:55dmoseursula: i'm not technically a peer, though i'm happy to review
14:59Mardaki can just make it replace the 30 line with
14:59Mardak+ 50 // NB: Bug 1381335 increased retries, so debug builds trigger idle in time
14:59dmose+1
15:00dmosethouh i bet a lot of people don't know what NB means, so you might consider just leaving it out
15:01ursulai agree with dan, i think the patch is good as is, as long as there's a comment in the bug that explains what's going on it's good enough to trace back why the change was made
15:03dmoseiactually meant "leave out NB"
15:03dmosei think the comment wants to stay in code
15:03dmoseso that it's easily discoverable on intermittent, not when somebody takes the time to decode the blame and figure out which bug it was etc
15:04ursulayeah good point
15:17Mardakdmose: so did you want me to change the comment here then? https://reviewboard.mozilla.org/r/157622/diff/3#index_header
15:18dmoseMardak: that comment looks fine to me
15:27Mardakfischer: thanks for the comment. (and thanks for staying up late! didn't mean to actually ping you :p)
15:28fischerMardak: not at all. Thank you for the help on this issue.
15:28Mardakdmose: looks like you just commented r=dmose instead of r+ from mozreview
15:33dmoseMardak: yeah, mozreview complained that i was reviewing squashed commits, told me about that problem, and then didn't tell me what to do about it.
15:33* dmose goes to poke again
15:34dmoseok, this time it got it
15:45as-github-botaaronrbenson: Hey! Someone just assigned you a PR for review: https://github.com/mozilla/activity-stream/pull/2867
15:48ursulaahh Mardak was gonna ask you if you wanted to do an export but you're too quick ;)
15:49Mardakursula: oh i might additionally remove dummy_test
15:49Mardaknot sure if it will complain.. hasn't complained before one pine though
15:50ursulaMardak: are you going to push this commit to try?
15:51Mardakwasn't going to. it's pretty much just like https://treeherder.mozilla.org/#/jobs?repo=pine&revision=bf23103ffd6e7167e25c05046eadd54830bf322a except with updated locales
15:52ursulacool
15:52as-github-botdmose: Hey! Someone just assigned you a PR for review: https://github.com/mozilla/activity-stream/pull/2660
15:53ursulaMardak: so do you want me to wait until you remove dummy_test?
15:56as-github-botursula: Hey! Someone just assigned you a PR for review: https://github.com/mozilla/activity-stream/pull/2869
15:56Mardakursula: updating mozreview too
15:58Mardaki guess i'll push the new export to try
15:58Mardakit's also different from pine as a-s is not enabled
16:23Mardakursula: not sure how it ended up selecting jared hirsch for the review.. i suppose he is the first item and maybe it was wrongly selected https://github.com/mozilla/activity-stream/pull/2869
16:48as-github-botMardak: Hey! Someone just assigned you a PR for review: https://github.com/mozilla/activity-stream/pull/2869
16:52ursulaMardak: ok feel free to land when try is happy
16:52as-github-botursula: Hey! Someone just assigned you a PR for review: https://github.com/mozilla/activity-stream/pull/2868
17:12as-github-botk88hudson: Hey! Someone just assigned you a PR for review: https://github.com/mozilla/activity-stream/pull/2859
17:48as-github-botAdamHillier: Hey! Someone just assigned you a PR for review: https://github.com/mozilla/activity-stream/pull/2859
17:49as-github-bottspurway: Hey! Someone just assigned you a PR for review: https://github.com/mozilla/activity-stream/pull/2610
17:52as-github-botdmose: Hey! Someone just assigned you a PR for review: https://github.com/mozilla/activity-stream/pull/2816
18:44as-github-botk88hudson: Hey! Someone just assigned you a PR for review: https://github.com/mozilla/activity-stream/pull/2859
18:52dmosehnmmm, some leakage in dt8 in the last two pine builds
18:52dmosehttps://treeherder.mozilla.org/#/jobs?repo=pine&selectedJob=114924196
18:52dmoseMardak: is that different from the one before?
19:37ursuladmose: that looks like it's the same issue that we were running into before with the other leak
19:37ursulasame platform too
19:37ursulabut it should have been fixed in central
19:37ursulais it possible that the pine build is behind?
19:41Mardakdmose: looks new? ursula: the previous devtool issue was a crash
19:41ursulai mean it was the same one as browser_blockHPKP.js
19:41ursulaMardak: https://github.com/mozilla/activity-stream/issues/2829
19:42Mardakhpkp fix was a browser/general test that standard8 happened to fix
19:44Mardakwhere his fix was just changing some bookmark api call in the test from sync to async
19:44Mardakso nothing specific about fixing the leak
19:46ursula:(
19:47Mardakdmose: the odd thing is the dt8 linux x64 debug passed with the "use dispatch avoiding Timer" and failed when removing dummy_test and updating HPKP list -- those two shouldn't cause dt8 to fail...
19:47Mardakretriggering to check..
19:51dmoseMardak: could be a race
20:22Mardakdmose: i pushed the enable patch to try and seems like windows 10 x64 debug has various failures ? are those important?
20:22Mardakhttps://treeherder.mozilla.org/#/jobs?repo=try&revision=790f8426d1641c96bdccce74f7f0c221fdfa5e00&group_state=expanded&selectedJob=114966087&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-classifiedState=unclassified
20:23Mardakursula/dmose: that devtools browser_json_refresh looks like it happens on osx debug too
20:27dmoseMardak: ugh. I'd be stunned if all of these were ours; I guess we'll have to go through them one at a time.
20:29as-github-botk88hudson: Hey! Someone just assigned you a PR for review: https://github.com/mozilla/activity-stream/pull/2868
20:45Mardaktspurway: https://searchfox.org/mozilla-central/search?q=early_beta_or_earlier
21:16Mardakdmose: so dt7/8 didn't fail on my try push...
21:16Mardakand got the r+ conditional of win 10 debug failures are actually intermittent
21:18dmosefair enough
21:39Mardakdmose: well... unfortunately those failures look pretty consistent
21:39Mardakhttps://treeherder.mozilla.org/#/jobs?repo=try&revision=790f8426d1641c96bdccce74f7f0c221fdfa5e00&group_state=expanded&selectedJob=114989885&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=pending&filter-resultStatus=running&filter-classifiedState=unclassified&filter-searchStr=win%20x64%20deb
22:05dmosedoh
22:05dmosei wonder why they haven't mostly been showing up on pine
22:05dmoseMardak: ^
22:05Mardakwindows 10 debug doesn't seem to run on pine
22:05Mardakwhy not? dunno
22:18* dmose blinks
22:23Mardakwindows 8 x64 debug does run
22:47Mardakdmose: SO
22:57mconleyMardak: GO GO GO GO GO
22:57mconley"You're all clear kid, now let's blow this thing and go home!"
22:58Mardakoh. bugzilla strips emojis :p it was supposed to be "I'M LANDING THIS!? "
22:59Mardaktime to change the desktop meeting notes i put in :p (maybe, hopefully)
23:00Mardakha ha ha ha searching for "windows 10" matches windows 8 because of e10s ;)
23:07Mardakmconley: do you know what happens if i include another commit that's already on autoland that's needed to resolve conflicts? i'm pretty sure rebase will just magically throw away the unneeded commit..
23:07mconleyMardak: hmmm
23:08mconleythat's a good question... uh
23:08mconleyMardak: definitely don't pull from autoland, I know that much
23:08mconleyoh
23:08mconleywait
23:08mconleyMardak: here's what I suggest:
23:09mconley1. hg update central
23:09mconley2. hg import <patch that you need to be based on>
23:09mconley3. hg rebase <activity stream enabling patch> on top of patch from 2
23:09mconley4. hg push -c <revision of _just_ the patch from 3 that enables Activity Stream>
23:10mconleythat&#39;ll push _both_ commits to the reviewing repository, but when autoland takes the patch that I r+&#39;d, it&#39;s only going to take that one commit and attempt to cherry pick it
23:10mconleyMardak: ^-- and that should apply cleanly.
23:10mconleysound good?
23:10mconleylet me know if you need more detail on that.
23:17Mardakmconley: got the gist of that. looks like mach is smart enough :) `git mozreview push head` (where i locally `git fetch hg::https://hg.mozilla.org/integration/autoland/` and `git cherry-pick`ed)
23:17mconleyoh yeah, you do the cinnabar
23:18mconley\_()_/
23:18mconleyI hope it works! :D
23:19Mardaki tried the usual `git mozreview push` and it provided a helpful:
23:19Mardakerror: cannot submit reviews referencing multiple bugs
23:19Mardakhint: limit reviewed commits by specifying a commit or revrange as an argument
23:20mconleyWOOOOO
23:20mconleyenablin&#39; is on the autoland
23:20mconleyif it sticks, tspurway&#39;s gonna have a nice welcome back gift
23:20Mardak
23:23mconleyand on that note
23:23mconleyg&#39;night
18 Jul 2017
No messages
   
Last message: 68 days and 6 hours ago