mozilla :: #activity-stream

17 Jul 2017
09:28as-github-botursula: Hey! Someone just assigned you a PR for review:
14:25as-github-botMardak: Hey! Someone just assigned you a PR for review:
14:32as-github-botpiatra: Hey! Someone just assigned you a PR for review:
14:35ursulaMardak: ping
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:38Mardakdefault parameters
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: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
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?
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:
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 except with updated locales
15:52as-github-botdmose: Hey! Someone just assigned you a PR for review:
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:
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
16:48as-github-botMardak: Hey! Someone just assigned you a PR for review:
16:52ursulaMardak: ok feel free to land when try is happy
16:52as-github-botursula: Hey! Someone just assigned you a PR for review:
17:12as-github-botk88hudson: Hey! Someone just assigned you a PR for review:
17:48as-github-botAdamHillier: Hey! Someone just assigned you a PR for review:
17:49as-github-bottspurway: Hey! Someone just assigned you a PR for review:
17:52as-github-botdmose: Hey! Someone just assigned you a PR for review:
18:44as-github-botk88hudson: Hey! Someone just assigned you a PR for review:
18:52dmosehnmmm, some leakage in dt8 in the last two pine builds
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: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: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: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:
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
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: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::` and `git cherry-pick`ed)
23:17mconleyoh yeah, you do the cinnabar
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:20mconleyenablin&#39; is on the autoland
23:20mconleyif it sticks, tspurway&#39;s gonna have a nice welcome back gift
23:23mconleyand on that note
18 Jul 2017
No messages
Last message: 8 days and 23 hours ago