mozilla :: #activity-stream

12 Sep 2017
00:10dmoseMardak: nice
04:01streaminatorncloudioj: Hey! Someone requested a review: https://github.com/mozilla/activity-stream/pull/3421
04:01streaminatoremtwo: Hey! Someone requested a review: https://github.com/mozilla/activity-stream/pull/3421
04:57streaminatorursula: Hey! Someone requested a review: https://github.com/mozilla/activity-stream/pull/3422
12:46streaminatorcsadilek: Hey, your patch was approved! https://github.com/mozilla/activity-stream/pull/3361
13:00Mardakk88hudson: might just be early or i just need to sleep a bit more.. but i believe you might have moved some code in pref changed observer for testing ?
13:44Mardaknanj: are tests looking good for rich icons? nit: `ok(domLinkAddedFired === 2, ` should probably be `is(domLinkAddedFired, 2, `
13:45nanjMardak: yes, finished two try runs last night and early this morning, all good
13:45nanjhttps://treeherder.mozilla.org/#/jobs?repo=try&revision=736966a39cb78d52fec40bf7644e1f3cfd324480&selectedJob=130258492
13:45nanjhttps://treeherder.mozilla.org/#/jobs?repo=try&revision=015dd3dc2408fcdc969652b454d25d7793220eb9
13:45nanjok, i
13:46nanj~ will fix that ok -> is
13:47k88hudsonMardak: whoops my rebase got messed up, sorry
13:47k88hudsonMardak: seems it was to early for *me*
13:48Mardakk88hudson: ;) for the notify, Services.obs.notifyObservers(null, "newtab-url-changed", ABOUT_URL); that's always going to notify with "about:newtab" and externally it it's always about:newtab withAS enabled or not.. hrmm
13:49k88hudsonMardak: yeah, that's just being used for tests right now
13:49k88hudsonit seems to be more for the overriding about:newtab stuff
13:49k88hudsonthat's why i added the second event
13:54Mardakk88hudson: newtab-url-changed happens when going from tiles to activity stream, so it doesn't seem that wrong for the same notification from activity stream to prerendered ? all of them are exposed as about:newtab
13:55Mardakwell i suppose the whole point of aboutNewTabService is that all urls are exposed as about:newtab :p
13:59k88hudsonyeah, that's true
14:03Mardaknanj: shall i push your patch to autoland?
14:05nanjMardak: yep, let's kick it off
14:09Mardaknanj: fyi, the assignee of the bug was ursula earlier. i updated it
14:11* nanj nod
14:26emtwok88hudson: nanj ahillier ursula tspurway is anyone interested in ramen today? :)
14:27nanjsure, count me in
14:34k88hudsonemtwo: i could do that
14:34Mardakmmm ramen
14:34emtwoMardak: if we had teleporters you'd be able to join us in time for ramen :P
14:38k88hudsonMardak: i don't think https://github.com/mozilla/activity-stream/issues/3371 is actually closed, right?
14:38Mardakk88hudson: welllll..... it doesn't generate a rtl file but it could ;)
14:38Mardakit has "support for rtl" :p
14:38k88hudsonMardak: ahhh right
14:39k88hudson:P
14:40Mardaki guess i can test what it looks like loading rtl because previous activity stream also assumed ltr
14:45Mardakk88hudson: ehh.. i suppose now with the placeholder text, you can more easily tell it's going from ltr to rtl
14:45dmosenanj: emtwo: do you have bandwidth for quick reviews on https://github.com/mozilla/activity-stream/pull/3421#issuecomment-328734511 this AM so that it lands before final export?
14:46nanjdmose: yes, we are doing the review right now
14:46emtwodmose: yea looking now
14:46Mardaklooking now!
14:46dmoseMardak: speaking of which, what's your thinking on how we playing chicken with autoland? export once around noonish with the most critical stuff, then again later with things that we hhope?
14:46dmoseMardak: speaking of which, what's your thinking on how we playing chicken with autoland? export once around noonish with the most critical stuff, then again later with things that we hhope?
14:47nanjwe were just talking about your change on Ping-centre, the logging change one
14:47dmosenanj: so i wasn't actually intended to land that one specifically
14:47dmosenanj: i just needed it in order to test
14:47dmosein theory the logStringMessage should be made to work right, but i didn't have time to dig into why it was busted locally and worked in nightlies
14:48dmosethat said, it might be ok to leave it in
14:48nanjdmose: okay, i am just going to review the second one
14:48dmosesoudsn good; i'll happilly strip the first one
14:49Mardakdmose: this early merge is more for initial feature freeze-ish i think. so changing aboutHome should probably happen as that's probably the most user facing change we have left
14:49Mardakcode can still land to m-c pretty easily after the first Merge Day
14:50dmoseMardak: oh, interesting. i was under the impression that everything was likely to be slushed to the point of requiring individual approvals after Weds. '
14:50Mardak"equire engg mngr approval for any fixes landing the week of 9/18 until 9/27
14:50dmoseoh, wow
14:50dmoseok, great
14:51dmoseMardak: so we will end up turning on both pre-render and about:home today?
14:51Mardakabout:home still has test failures. ursula was looking at it i believe?
14:52Mardakbut prerendering is ready to turn on
14:52ursulayeah i was looking at the failure in browser_aboutHome.js but got sidetracked with metadata. was going to get back to it today
14:52dmoseMardak: does turning on prerendering requiring any github work, or just kate's m-c patch?
14:52ursulabut we should turn prerendering on and turn abouthome on and push to try to see what new try failures there are
14:53Mardakdmose: i'll need to revert the final m-c-patches diff but pretty much just the m-c patch
14:54dmoseMardak: so it can be turned on without a new export?
14:54Mardakyup
14:54Mardakwell we already exported it yesterday
14:55dmoseMardak: k88hudson: hawt. it seems like there's value in getting it turned on on m-c ASAP so that there's as big a gap in time as is possible between turning that on and the about:home export
14:56dmoseso there's as much time as possible for talos to accumulate data with prerendering but without about:home
14:56dmoseso that it's clearer what timing changes should be attributed to what code changes
14:58nanjemtwo: i think this is what i flipped last time to enable the logging "devtools.chrome.enabled"
14:59emtwonanj: do you have to restart the browser for it to take effect?
14:59emtwoif doesnt change whether i see logging or not for me
14:59nanjemtwo: no, i didn't restart the browser
15:15streaminatorMardak: Hey, your patch was approved! https://github.com/mozilla/activity-stream/pull/3422
15:15streaminatorMardak: Hey, your patch was approved! https://github.com/mozilla/activity-stream/pull/3422
15:32streaminatorMardak: Hey! Someone requested a review: https://github.com/mozilla/activity-stream/pull/3427
15:33k88hudsonMardak: want to do a sanity check on https://reviewboard.mozilla.org/r/178400/diff/4-5/
15:37Mardakk88hudson: looks fine. autolanding!
15:37k88hudson
15:38Mardakk88hudson: (i suppose i should note that technically if a-s is not enabled but prerender pref changes, about:newtab doesn't actually change)
15:39k88hudsonMardak: true, that notification means different things anyway though
15:40k88hudsonthe file would probably be easier to read/refactor if the defaultUrl and actual overriding stuff was somewhat separate
15:40k88hudsonsince they are actually totally separate things
15:40k88hudsonwell, mostly separate
16:19dmosek88hudson: got a few mins to chat re the client-side telemetry code structure?
16:39k88hudsondmose: yep sure, your vidyo
16:39k88hudson?
16:42dmosek88hudson: sure
16:58Mardakr1cky is looking for an issue to fix! anyone have something to share?
16:58Mardakr1cky: maybe the autofocusing? https://github.com/mozilla/activity-stream/issues/2007
16:59r1ckyo/
16:59Mardakthe thing is the location bar will try to take focus, so we might just need to take it back. but only for about:home where we can check via content-src/lib/constants IS_NEWTAB
17:00Mardakoh. and it needs a pref so we can toggle via shield study
17:00k88hudsonhey r1cky!
17:00tspurwayhey r1cky. welcome back!
17:00r1ckyheyo! lol. yay
17:01ursulaglad to hear you're ok r1cky :D
17:03r1ckythanks! was crazy but everything ok. we got lucky it moved west. i guess unlucky for others that were on that side
17:13Mardakahillier: hrmm. on my set of highlights with image caching, it's running into JavaScript error: resource://gre/modules/PageThumbUtils.jsm, line 142: Error: Image has zero dimension
17:13Mardakthen it stops fetching any more thumbnails
17:13ahillierMardak: that's odd can you tell which image is causing that issue?
17:13Mardakthis is the first one with the gradient https://feedly.com/i/latest
17:16Mardakahillier: oh i suppose it would be good to note that that url is bookmarked
17:16Mardakso we're probably falling back to screenshot
17:17ahillierAh okay
17:17ahillierHmm
17:17Mardakalthough i see we just do getScreenshot(imageUrl || url)
17:18Mardakwithout the explicit {isImage} or whatever that flag was
17:18ahillierYeah
17:18ahillierIt's the exact same code as used with TopSites
17:19ahillierBut that zero dimension error is in the imageThumbCanvas function (or whatever it was called) so that means that it is being detected as an image
17:20ahillierSo I guess not a screenshot?
17:20MardakImage has zero dimension: http://s3.feedly.com/img/feedly-512.png
17:20ahillierAh
17:20Mardaklooks like width and naturalWidth should both be 512...
17:21ahillierYeah
17:21ahillierThat's what I get
17:21Mardakfyi, that feedly-512.png is indeed the previewImageURL value
17:21ahillierUnless maybe in the background page for some reason the image didn't load properly?
17:22ahillierAh okay
17:22Mardakso i guess 2 issues: 1) why is it getting 0 width, 2) why is thumbnailer not continuing after getting this error
17:23ahillierIt should just be continuing, yes, that's potentially the more concerning of the two...
17:23ahillierI can reproduce the error with that site
17:32Mardakahillier: ha ha ha............ if we do if (!image.complete || image.width === 0). the second load gets it ................
17:33nanjdmose: shall we do a quick vydio chat to sort out the work for the "page" change?
17:33ahillierMardak: bizarre, but nice catch
17:34Mardakahillier: uh..... so from the content side, the width and height are 512
17:34Mardakbut are 0 from the jsm
17:34ahillierIt would be nice though if thrown errors didn't disrupt subsequent captures
17:34dmosenanj: can we do it in 30 mins or so?
17:34ahillierYeah that's weird
17:35nanjdmose: sure, let me know when you're ready
17:35dmosenanj: will do
17:36ahillierMardak: so need an m-c patch?
17:37Mardakahillier: probably. but still need to figure out what's the actual issue
17:38Mardakahillier: got it
17:38Mardakahillier: image.src === https://s3.feedly.com/img/feedly-512.png. url === http://s3.feedly.com/img/feedly-512.png
17:38Mardakwe requested http. page redirected to https
17:39ahillierOh...
17:39Mardakthe logic says if the url doesn't match, create a new img element. that image element is always .complete==true
17:42ahillierRight, so do we want to check for url minus protocol match, or just add in the extra if guard?
17:44ursular1cky: question for you
17:45ahillierMardak: And do a try/catch in backgroundPageThumbsContent._captureCurrentPage
17:46Mardakahillier: well in this case it happens to redirect to the same url except protocol, but it could have very well just redirected to a different url.. in which case we should still just save the image?
17:46ahillierMardak: Yeah makes sense
17:47Mardakahillier: yeah try/catch calling _failCurrentCapture it seems
17:47r1ckyursula: what's up
17:48ursulaso i'm looking at ticket 3428
17:48ursulawhere if you pin something the icon disappears, and that's because it's not copying over the data from the original link
17:48ursulaso i was looking at this: https://github.com/mozilla/activity-stream/blob/master/system-addon/lib/TopSitesFeed.jsm#L132
17:48ursulais there any reason this can't be: Object.assign(pinnedlink, link.find(...) || {hostname})
17:49ursulaso that you're copying over the data onto the pinned link?
17:49ursulathat seems to fix the bug but i don't know if that changes something with the deduping or something
17:49r1ckygood question
17:49ursulait passes all the test so
17:50r1ckyso pinnedLink I think only has a url and a label? so it *should* be fine. what is it overwriting though?
17:50ursulait's swallowing up the favicon, and the faviconSize
17:50r1ckyo.0
17:51r1ckythat's odd. but yeah, dont see how reversing it would break anything
17:51ursulacuz NewTabUtils.pinnedLinks.links doesn't return the places entry right it just returns a list of urls
17:51ursulaso we need to smush on the original link's data
17:51r1ckyyeah it returns whatever is in the pinned links pref
17:51r1ckyand we only put {url,label} in there
17:52ursularight
17:53ursulaso i can make that change then?
17:54r1ckyursula: go for it!
17:54ursulathanks :D
17:54r1ckywhat could possibly go wrong :-)
17:54ursulai guess we'll find out
17:57k88hudsonwho wants to review prerender supporting highlights etc.
18:01k88hudsonMardak: r1cky: dmose: ursula ^ whose got time https://github.com/mozilla/activity-stream/pull/3430/files
18:02streaminatorMardak: Hey! Someone requested a review: https://github.com/mozilla/activity-stream/pull/3430
18:02k88hudsonty
18:05Mardakk88hudson: somewhat unrelated, how should feeds be checking prefs? i see some places doing new ActivityStreamPrefs()
18:05Mardakfor NSFW stuff i ended up doing store.getState().Prefs.values
18:05k88hudsonMardak: they should be looking at store.getState() in general yeah
18:09Mardakk88hudson: ok filed https://github.com/mozilla/activity-stream/issues/3431 for later
18:09k88hudsonMardak: sounds good
18:10k88hudsonhave we filed changing ActivityStreamPrefs to use Services.Preferences instead of Preferences.jsm
18:10k88hudsonthat shoudl also be done at some point as well
18:17dmosenanj: now could work
18:18dmosenanj: your room?
18:18nanjdmose: yep
18:18mconleyk88hudson: ping
18:18k88hudsonmconley: pong
18:19mconleyk88hudson: hiya! I'm talking with an engineer from the Firefox Core Engineering team working on reducing the number of modules that we load on start-up. One of those modules is RemotePageManager.jsm, and he's made some good progress on making it lazier
18:19bytesizedhello!
18:19mconleyk88hudson: however, his patch seems to cause Activity Stream to not load, and he's looking for someone to talk to about how Activity Stream uses RemotePageManager
18:19mconleyk88hudson: aaaaand so, let me introduce you to bytesized!
18:20bytesizedk88hudson: hi! I have been poking around trying to figure out how things work
18:20k88hudsonbytesized: hi! yep, I can help
18:20mconleybytesized: k88hudson knows all the guts of Activity Stream, and can help point you in the right direction.
18:20bytesizedgreat!
18:21bytesizedk88hudson: I'm not really sure where I should start. Right now, I am trying to have RemotePageManager.jsm loaded later. but when I do that, these callbacks never get called:
18:21bytesizedhttp://searchfox.org/mozilla-central/rev/51eae084534f522a502e4b808484cde8591cb502/browser/extensions/activity-stream/lib/ActivityStreamMessageChannel.jsm#133-135
18:21bytesizedk88hudson: I am still trying to figure out why
18:25bytesizedk88hudson: Since RemotePageManager is imported within that file, I cannot figure out how changes to when other files load it would affect it
18:26bytesized(I did not change how Activity Stream loads it, just toolkit/content/process-content.js)
18:26bytesizedk88hudson: Although even just a high level description of how Activity stream uses remote pages would be great
18:27bytesizedstill pretty new to this area of code
18:27k88hudsonbytesized: so in ActivityStreamMessageChannel generally the existing RemotePages instance is passed down to Activity Stream
18:27bytesizedoh
18:27bytesizedthat is good to know
18:28k88hudsonbytesized: activity stream uses it for message passing from the main (stuff initialized in ActivityStream.jsm) and in content on about:newtab/about:home
18:29bytesizedhmm, alright
18:29k88hudsonbytesized: we
18:29k88hudsonoops
18:30k88hudsonbytesized: we've noticed problems if there are existing pages around *before* RemotePages gets initialized because they'll never get attached, if i remember correctly
18:30k88hudsonif i recall correctly, is that right ahillier Mardak? ^
18:30bytesizedk88hudson: hmm. I don't *think* that is happening, but that is good to know
18:31k88hudsonok
18:32MardakRemotePageManager gets loaded pretty early on via AboutNewTab
18:32k88hudsonyeah, so that fixed the issue with it not attaching
18:32MardakActivityStream leverages that fact so that the addon can be delayed in loading and still get any early about:newtab/about:home pages that register before the add-on
18:33k88hudsonI imagine in might be tricky to delay the loading of RemotePageManager once we're loading on about:home (which is happening imminently) without the ability to attach pages after the fact
18:33bytesizedhmm
18:34k88hudsonalthough attaching slightly later would theoretically be OK, if that were supported
18:36k88hudsonwhoa has anyone seen the new line reference UI in github? cool
18:36bytesizedk88hudson: I am going to lunch, but I will let you know if I have any questions. Thanks for your help!
18:36k88hudsonbytesized: ok ping us anytime!
18:36bytesizedk88hudson: thanks! will do
18:36Mardakk88hudson: only for a fixed commit revision (hit "y")
18:38k88hudsoncool!
19:14streaminatorr1cky: Hey! Someone requested a review: https://github.com/mozilla/activity-stream/pull/3434
19:17ursulaare we doing an export today Mardak/k88hudson?
19:17Mardakwe could
19:17Mardaktwice a day until final freeze?! ;)
19:18ursulalet's do it!!
19:18ursulahahaha
19:18k88hudsonwe definitely should
19:18ursulai've got a top sites bug in review that i'd like to get in today
19:19streaminatorursula: Hey, your patch was approved! https://github.com/mozilla/activity-stream/pull/3434
19:19Mardakwell i filed this as the placeholder bug https://bugzilla.mozilla.org/show_bug.cgi?id=1399226
19:20ursular1cky: wow speedy
19:20ursulathanks!
19:20Mardakursula: well hold on ;)
19:21Mardakpinned links are just json in a pref. i suppose we wouldn't want to put that data into the pref? it contains data uris?
19:21k88hudsoni'd like to get the highlights-related prerendering stuff in as well yeah
19:21ursulado we keep pinnedlinks in our own pref??
19:21Mardakoh you already merged :p
19:21ursulai can revert LOL
19:22Mardakno we store pinned data in the browser.newtabpage.pinned pref just like tiles
19:22r1ckywait wat? dont think that patch changed what was being stored
19:22ursulaoh yeah then that should be fine
19:22ursulayeah what ricky said
19:22Mardakright it doesnt. i was just thinking if we should be storing the data in the pref in case we don't get the data from places
19:22ursulaahh
19:22ursulayeah it contains data uris
19:22r1ckyohhh
19:23Mardakyeah probably don't want too many data uris in the pref ;)
19:23ursulawe don't love data uris THAT much
19:24ursulaso, for browser_aboutHome.js ... i'm going to try to fix the search tests so that they actually work for our search bar
19:24ursulabut if i can't get that to work, should we just turn activity stream about home off for this test?
19:24k88hudsonoh yeah what's left for about:home
19:25k88hudsonanyone have a recent try push
19:25ursulanope, was waiting for prerendering to land
19:25Mardakprerendering on by default is in autoland, so you can grab it from there and include it in your try push
19:26k88hudsoni can push it
19:26Mardakshould be able to --artifact
19:26k88hudsonnah it has idl changes
19:26k88hudson:'(
19:27k88hudsonalthough those are just for tests...
19:27k88hudsonso yeah actually --artifact would be fine, just keeping in mind those tests will fail
19:28r1ckyMardak: how do i get about:home to be AS or is that not in yet?
19:28Mardak./mach run --setpref browser.newtabpage.activity-stream.aboutHome.enabled=true
19:32r1ckyMardak: so... right now when the browser loads, the awesomebar is focused. isn't that some m-c code that must be doing that? the content can't steal that focus
19:33Mardakr1cky: oh.... ha.............
19:33Mardaknod i see setInterval(() => document.querySelector("input").focus(), 1000) doesn't steal focus
19:35Mardakr1cky: this is trickier than i thought. might need some chrome help to focus the browser.. hrmmmmmmm.....
19:35r1ckyMardak: but the old about:home does it
19:35Mardaknot any more ;)
19:36Mardakhttps://bugzilla.mozilla.org/show_bug.cgi?id=1395961
19:36Mardakas noted in that comment 0, it was "regressed" by https://bugzilla.mozilla.org/show_bug.cgi?id=1393802
19:38k88hudsonhttps://treeherder.mozilla.org/#/jobs?repo=try&revision=0a72b0051aec4998840f5bdc7e16d1f88370faa4
19:38Mardakr1cky: nod if i do setTimeout(() => gBrowser.selectedBrowser.focus(), 5000) in addition to the querySelector("input").focus(), the search box gets focused
19:39Mardakr1cky: with a window.addEventListener("focus") i see a focus event { target: window }
19:39Mardakthat's when chrome focuses the browser from the location bar
19:40Mardakalthough if the page already would have had focus in the search box, it will be correctly focused when focus moves from location bar to the browser
19:41r1ckyyeah
19:41Mardakso.. i suppose we can focus the search box and dispatch something to get the browser focused
19:44k88hudsonursula: http://searchfox.org/mozilla-central/source/browser/modules/test/browser/browser_UsageTelemetry_content_aboutHome.js#73
20:03streaminatorMardak: Hey! Someone requested a review: https://github.com/mozilla/activity-stream/pull/3435
20:07k88hudsonMardak: rlr: want me to take that one ^?
20:07k88hudsonMardak has a lot of r's looks like
20:07k88hudsonalso WOW looks way nicer
20:14r1ckyk88hudson: sure! thanks!
20:14r1ckyoh yeah, Mardak's face is all over the pulls page heh
20:15streaminatork88hudson: Hey! Someone requested a review: https://github.com/mozilla/activity-stream/pull/3435
20:16streaminatorr1cky: Hey, your patch was approved! https://github.com/mozilla/activity-stream/pull/3435
20:16streaminatorr1cky: Hey, your patch was approved! https://github.com/mozilla/activity-stream/pull/3435
20:31streaminatork88hudson: Hey, your patch was approved! https://github.com/mozilla/activity-stream/pull/3430
20:31streaminatork88hudson: Hey, your patch was approved! https://github.com/mozilla/activity-stream/pull/3430
20:34Mardakk88hudson: got to use that higher order function capability of javascript! >:P
20:35Mardakr1cky: eh? where did that reddit icon come from?
20:35Mardakthere's this with transparent background https://www.redditstatic.com/icon.png
20:36Mardakand this with blue burst background https://www.redditstatic.com/icon-touch.png
20:36Mardaki don't see the circular red background one..
20:40r1ckyMardak: not sure. bryanbell and k88hudson were diving into manifest files and also spoofing user agent to get mobile icons
20:41Mardakand youtube.. slacking on their logo change :p
20:59dmosenanj: i'm going to spin off the n/a stuff to another bug
20:59nanjdmose: yep, sounds good
21:01ursulak88hudson: right now the behavior on about:home is if you click somewhere on the page and then start typing a key in the keyboard the searchbar gets autofocused and starts typing the thing
21:01ursulado we still want that behaviour?
21:01ursulahttps://dxr.mozilla.org/mozilla-central/source/browser/base/content/test/about/browser_aboutHome.js#460
21:01k88hudsonI think we ruled that we would not do that for 57
21:01k88hudsonlike it was a nice to have
21:01k88hudsonwe had an issue for it
21:01ursulaoh yeah?
21:02k88hudsonyeah i'm pretty sure
21:03dmosenanj: emtwo: ok, new stuff up for review.
21:03nanjdmose: on it!
21:04dmoserockin; thanks!
21:05ursulaworks for me
21:07Mardakursula: was to be disabled here https://github.com/mozilla/activity-stream/issues/3252 but i guess 3302 took over
21:07Mardakursula: restoring that functionality later here https://github.com/mozilla/activity-stream/issues/3264
21:07ursulaMardak: oh i see
21:07ursulaperfect
21:07ursulathanks!
21:09streaminatordmose: Hey, your patch was approved! https://github.com/mozilla/activity-stream/pull/3421
21:09streaminatordmose: Hey, your patch was approved! https://github.com/mozilla/activity-stream/pull/3421
21:09dmosehawt
21:12dmoseis there a problem with including an automatically updated package-json.lock in a PR for other stuff?
21:12dmoseer package-lock.json
21:13k88hudsondmose: what's causing the change
21:13dmosegot me
21:13dmosei did update npm to 5.4.1
21:13dmosemaybe that's related
21:13k88hudsonah that could be why
21:13dmosei can either include the changes
21:13dmoseor strip them out
21:15* dmose wonders why nvm isn't sticking with the old npm automagically
21:15k88hudsondmose: justa sec let me take a look
21:16k88hudsondmose: yeah looks like implementation details caused by the npm upgrade
21:17dmosek88hudson: so strip them out?
21:17k88hudsonI'm not sure what to do about that one, cause we might get some churn from people on other versions
21:17k88hudsoni'm leaning towards stripping them out yeah
21:18dmoseyeah, and i'll try and figure out how to force nvm to Do The Right Thing
21:18* dmose is truly annoyed at how non-trivial it is to pin node/npm versions
21:18k88hudsonyeah tell me about it
21:19dmoseit's extra-special that they actually made it harder by deprecated the engines strict thing recently
21:19dmoseand moving it to individual user settings
21:19dmosedeprecating
21:19k88hudsonengines strict is the worse though
21:19k88hudsonworst
21:19dmosereally?
21:19dmoseseems liek it'd be better
21:19dmoseor at least more predicatable
21:19k88hudsonyeah cause it breaks peoples builds in a way they can't fix sometimes
21:20dmosegood times
21:20k88hudsonlike for example if you have a subdependency with engine-sctrict that doesn't actually get used in the part of the dependency you are using
21:20dmosehahaha
21:20k88hudsonit prevents you from installing anything at all
21:20dmoseoh yeah, that'd be special
21:20dmosedidn't think about that
21:20dmosedependencies are hard
21:20dmosei'm going shopping
21:20k88hudsonyeah, basically
21:20k88hudsonlol
21:22dmosek88hudson: would switching to yarn make any of this stuff suck less?
21:22dmosek88hudson: or would it just be trading one set of issues to another
21:23k88hudsondmose: I'm biased but I think yarn would probably worse
21:23k88hudsonbut i haven't used it beyond playing around for a few minutes
21:24k88hudsonso if anyone has some compelling reasons to try it i'd be open to it
21:24k88hudsoni also don't like the idea of using github as a registry
21:28dmoseyeah, requiring an explicit publish step does have some value
21:28streaminatorahillier: Hey, your reviewer requested some changes: https://github.com/mozilla/activity-stream/pull/3427
21:28streaminatorahillier: Hey, your reviewer requested some changes: https://github.com/mozilla/activity-stream/pull/3427
21:34streaminatork88hudson: Hey! Someone requested a review: https://github.com/mozilla/activity-stream/pull/3438
21:48dmosepine is back to a relatively happy place again
21:49dmoseso browser through the treeherder results for your recent pushes is worth a few minutes
21:49dmoseer browsing
21:50dmosei had to skip a bunch of pushes from yesterday, though
21:58k88hudsondmose: can you think of a good check for being in main v.s. content other than Components && Components.utils
22:02dmosek88hudson: hmm
22:02k88hudsondmose: actually i'm stealing what you did before
22:03k88hudsoni.e. typeof Window === "undefined";
22:03k88hudsonMardak: dmose: want to do an export
22:03k88hudsonsoon
22:03dmosek88hudson: that'll tell you whether you're in content vs. a JSM
22:04k88hudsonyeah that's fine
22:04Mardakk88hudson: i think ahillier is close (although maybe away now?)
22:04dmosek88hudson: as long as you're not trying to use the module in a chrome window, you should be ok
22:04k88hudsonah right
22:04k88hudsonnope he's here
22:04ahillierI'm still around
22:04ahillierAbout to push changes
22:04Mardakok irc says away for some reason :p
22:04k88hudsonanyone know what the status is on those about home test failures? ursula is gone
22:04Mardakoh /back ;)
22:06streaminatorahillier: Hey, your patch was approved! https://github.com/mozilla/activity-stream/pull/3427
22:11streaminatordmose: Hey! Someone requested a review: https://github.com/mozilla/activity-stream/pull/3439
22:14streaminatork88hudson: Hey, your patch was approved! https://github.com/mozilla/activity-stream/pull/3439
22:19dmoseMardak: if we want to get it into the export, it would probably be reasonable to merge the page: about:home patch, even though emtwo hasn't re-reviewed
22:19dmoseshe was pretty happy with the first iteration, and nan did re-review
22:19dmoseMardak: that said, it can prob wait until tomorrow too
22:19emtwodmose: oh I did review it. just figured nanj 's r+ was sufficient
22:20dmoseoh, ok!
22:20emtwoit looks good!
22:20dmoseeven better!
22:20dmosethanks!
22:20dmoseMardak: i'll go strip out the console.log
22:20dmoseand then merge it
22:20k88hudsonare we waiting til tmr to pref on about:home
22:20dmoseif the tests are still in bad shape, presumably we need to?
22:20dmosebut
22:20* dmose defers to mardak on this one
22:21k88hudsonwe could just turn those tests off
22:21k88hudsonfor now
22:21k88hudsonand restore them as needed
22:21dmosetrue
22:21Mardakk88hudson: the export and turning on about home are probably two separate bugs to land
22:21k88hudsonit looks like the browser_aboutHome tests *might* be the only ones
22:21k88hudsonMardak: yeah definitely
22:21k88hudsoni'm just wondering if we want to do that now v.s. later
22:22k88hudsonhttps://treeherder.mozilla.org/#/jobs?repo=try&revision=0a72b0051aec4998840f5bdc7e16d1f88370faa4&selectedJob=130436680
22:24dmoseMardak: merged page: about:home
22:53Mardakdmose: it looks like we're good to export?
22:54dmoseMardak: works for me
23:16dmoseMardak: ok, i haven't actually tested, but i can do that if you think it'd be helpful
23:16dmoseMardak: I've put r+ in the bug for a couple of nits that don't block
23:26dmoseMardak: i just skimmed some of the recent stuff on pine; so far none of it is obviously ours
23:26dmosei starred everything that i skimmed
23:27Mardakok trying it locally as well. it's not directly dependent on any other autoland stuff but it would be better with the thumbnails fix
23:27dmoseat this moment, much of the remaining oranges and reds look like DUPs of those
23:27dmoseMardak: which thumbnails fix?
23:27Mardakhttps://bugzilla.mozilla.org/show_bug.cgi?id=1399200
23:28Mardakit's already in autoland queue but tree closed
23:28dmosewell, if it's already in autoland, it seems highly like that we're good
23:28dmosesince this export will be behind it in autoland
23:29Mardaknod
23:40Mardakdmose: trying latest autoland with latest a-s master with my usual profile..
23:40Mardaki have 5 high res icons, 4 corner icons, 3 with just screenshots
23:41Mardakdo we want that type of telemetry?
23:46dmoseMardak: i don't understand the questiohn
23:46dmoseMardak: are you asking if we should add code to give that back to us as telemetry?
23:46Mardakis it something discussed before and tracked as an issue?
23:47dmoseMardak: i'm not aware of that having been discussed and tracked
23:48dmoseMardak: i think it would be excellent info have telemetry about to help us direct our effort
23:54Mardakthere we go. finally got all 12 top sites to have rich icons :p
23:55dmoseyikes
23:55dmoseshould probably file a short-term bug with design feedback needed to see if we can brainstorm a way to make this better before 57
23:56Mardaklooking at places. my 70th most frecent site gets me the 12th rich favicon top site ;)
23:57dmoseheh
13 Sep 2017
No messages
   
Last message: 9 days and 50 minutes ago