mozilla :: #activity-stream

7 Sep 2017
00:13streaminatorMardak: Hey, your patch was approved!
14:09r1ckyMardak: so now I can use newtabutils to get highlights?
14:09r1ckyif i pull latest m-c
14:27tspurwaythe answer is yes, r1ckyf
14:29tspurwayhows the Highlights UI looking, r1cky?
14:30r1ckytspurway: this was from yesterday using the queries copy pasted into my code
14:30r1ckynow need to figure out how to use the queries that landed
14:30tspurwaythat looks great, r1cky
14:32k88hudsonlooking good r1cky!
14:43ursular1cky: nice! just heads up, the patch to actually collect metadata hasn't landed yet (will land today) so description and preview image will be null coming out of the db for all sites
14:50r1ckyursula: ok cool. Ed gave me a little browser console scripty to populate some for testing
15:13ursulaLOL the snippets
15:13ursulalooks awesome though r1cky :D
15:13ursulathose highlights
15:13r1ckyhaha i cant get rid of that snippet bar
16:46k88hudsonr1cky: are you doing placeholders in the highlights stuff
16:49k88hudsondmose: I guess we removed the mozilla-central-patches task?
16:50k88hudsondidn't we have one at some point?
16:50dmosek88hudson: last i saw, it was still there
16:51dmoseeven though there were no patches at that moment
16:51dmosei had to make a fix for that work :-)
16:51k88hudsonwhat was the task again
16:51dmosek88hudson: i don't see it either
16:52k88hudsonor is it just part of that mochi tests setup task
16:53dmosei don't see a commit removing it
16:53dmosewell, mochitests does it
16:53dmosebut we had npm tasks to do it too, i think
16:53k88hudsonif the folder is empty that makes sense
16:53k88hudsonyeah i thought we had a task
16:53dmosewell, my assumption was that we wanted to leave the infra in-place
16:53dmosesince we'd likely have to re-add patches from time-to-time
16:54k88hudsondmose: anyway, I'm adding one now
16:54k88hudsonfor the URL switching
16:54k88hudsonI've added a bunch of js docs
16:54k88hudsonas well
16:54dmosesounds good
16:59r1ckyk88hudson: i'm using Sections and it looks like it has placeholders to fill the row
16:59k88hudsonr1cky: oh right! that makes sense
16:59k88hudsonawesome :D
16:59r1ckyyep, got that for freeee
17:01Mardakr1cky: it'll be await NewTabUtils.activityStreamLinks.getHighlights()
17:02r1ckyMardak: yep. already got it working :)
17:02Mardaki dont think it made it to today's nightly though but latest m-c should have it
17:03r1ckyyeah, i pulled latest m-c and it worked. but didnt see it on searchfox
17:58dmosek88hudson: ok, i've reviewed the first 13 commits! it's looking great.
17:59k88hudsondmose: i've added quite a bit of documentation and stuff
17:59k88hudsonand some more tests
17:59dmoseyep, i saw
17:59k88hudsonthank you!
17:59dmoseanyway, before landing, i'm assunming the things that still need to be done are
17:59dmose* finish addressing review comments
17:59dmose* file bug for changes to m-c side of the house
18:00dmose* make a list of the spin-off bugs
18:00dmosei would like to at least skim the remainder of the review fixes
18:00dmoseand the list of spin-off bugs
18:01dmoseand i need to go meet someone; i'll be online more to do that stuff after i get back
18:09k88hudsonsounds good, thank you!
18:17Mardakthe m-c side of newTabServices change can land after we land in github as m-c just won't know to use -prerendered.html but still correctly handle original case
18:26Mardakursula: if metadata collection sticks, we should be able to export r1cky's highlights work and maybe get highlights in tomorrow's nightly with images and description?
18:27ursulaMardak: sounds like a plan to me
18:30Mardakursula: did it sound like the better handling meta bad tags was for 57? worst case right now is that the meta tag throws an exception in the handler and skips the tag?
18:30ursulaMardak: i don't think it's for 57. the data expiry is 57 for sure
18:37k88hudsonMardak: yep that's right, it can land after
18:39Mardakursula: i locally rebased/built your autoland patch and NewTabUtils.activityStreamLinks.getHighlights().then(v => console.log(v)) is working ! :)
18:39Mardakr1cky: got a WIP highlights patch/PR to test?
18:39ursula yaaaaa!
18:41* Mardak wonders how long until someone files a bug/issue that bookmark library description doesn't get used in highlights
18:43r1ckyMardak: sure. i'm still working on tests but i can throw up a WIP PR
18:43Mardakyessshhhh please
18:48Mardaki haz highlights!
18:55Mardakursula: hmmm. this page didn't seem to auto add metadata
18:55Mardaki was able to add it manually
19:02ursulaMardak: give me a min just debugging something with nan and i'll take a look
19:21ursulaMardak: odd, i was able to add it with the patch locally applied
19:21Mardakursula: i think i know what's going on
19:22ursulaMardak: what?
19:23Mardakursula: not telling!
19:23Mardak:p saving output :p
19:25ursuladid i break something when i made the review changes? :(
19:25k88hudsonwhoa looking good!
19:26k88hudsonMardak: is that with the image caching stuff
19:26Mardakursula: first part is the dumping i added and below is output
19:26k88hudsonyour screenshot
19:26Mardakk88hudson: no. live fetching of images
19:26k88hudsonah ok
19:26Mardakhot linking i should say :p
19:26k88hudsonright makes sense
19:26Mardakursula: i went to 3 sites. first site it sends meta while it's still getting lots of meta
19:27Mardakbut that one is okay
19:27Mardakursula: second and third never print out the entry. where reddit has a description but failed somewhere
19:28Mardakoh and i just noticed. about:newtab has meta tags too that get processed... oh well :p
19:30ursulaso what happens to the second and third site?
19:30Mardaknot sure yet ;)
19:30Mardakadding more debugging!
19:33Mardakursula: uho..
19:33Mardakmeta: entry {"description":{"value":"reddit: the front page of the internet","currMaxScore":1},"image":{"value":"","c
19:33Mardakthat run i loaded reddit first then the news article
19:34ursulathey're getting mixed up?
19:36Mardakursula: if i load electrek first then reddit, shouldExtractMetadata returns false
19:36Mardakprobably because it has the entry from electrek and decides "description" is lower ranked than what already exists
19:40Mardakursula: ha ha ha. DEFAULT is being changed somehow
19:40ursuladefault is being updated to what entry is?
19:43ursulaMardak: noooooooooo it just got backed ouuuuut
19:44ursulawhich i guess is ok since we're trying to fix it anyways
19:47Mardakursula: seems like at minimum we should increase the 100ms timer delay
19:47Mardaknot sure if that's causing the test failure
19:51Mardakursula: OHHH duhhhhhhhhhhhhh
19:51Mardakyou have objects within DEFAULT_MAP_VALUES
19:51Mardakthose object references are copied even with Object.assign
19:51Mardaksimple fix is to have DEFAULT_MAP_VALUES shallow
19:52Mardakdescription: null, descriptionScore: -1
19:53ursulaoh shit
19:53ursulai see
19:56Mardakursula: or just define the object within the callback instead of top level scope ;)
19:56Mardakskips the Object.assign step
19:58ursulaMardak: i think we should just define the object within the callback
19:58Mardaknod. avoids someone else running into this same problem
19:58Mardak"why not refactor and have an image sub object!" :p
19:58* ursula eye twitch
20:00ursulaMardak: what arbitrary number shall i try next for the timeout?
20:00ursulawho knows
20:00Mardakursula: i'll try to measure for a few sites from first timeout to last meta
20:05ursulaMardak: should i increase the interval at which the waitForCondition polls?
20:05Mardakhow is the test failing?
20:05ursulapreviewimageurl is null apparently
20:06ursulaprobably cuz i'm trying to access .href on previewimageurl which is null but
20:06ursulait shouldn't be, since the waitfor condition is that it exists
20:08Mardakursula: for timer, i&#39;m seeing electrek has a statically defined <meta> that appears 274ms after getting a first desired meta tag. i believe this delay is from <script> synchronous processing in <head>
20:08ursulayup that makes sense
20:08Mardakhow about... 1 second? :p
20:09ursulaworks for me ;)
20:09ursulachanging the license while i&#39;m here too
20:11ursulai&#39;ll have to change the polling interval to be 1000ms too
20:12Mardakursula: if waitForCondition timed out, it would have thrown and caused a different test failure
20:12Mardakursula: i believe increasing the timer will fix this but you should check previewImageURL instead of previewImageURL.href
20:13Mardakthe description tag probably triggered the timeout and set page info for description and no image
20:15ursulaMardak: so 1. increase delay to 1000ms 2. put default values in the callback 3. change the license 4. change waitforcondition to return previewimageurl instead of previewimageurl.href
20:21Mardakursula: sounds good. and hopefully the debug machines aren&#39;t /that/ slow..
20:23Mardakursula: ohhh hey :p the test failed locally
20:24Mardakit looks like it&#39;s failing not because description updated too soon. it&#39;s because it never updated :)
20:24ursulaMardak: really? on your mac?
20:24Mardakit&#39;s because checkForCondition is checking too soon ;)
20:25ursulaso we should increase the polling interval?
20:26Mardakshouldn&#39;t be necessary
20:27Mardakwith a longer 1sec timer delay, there&#39;s more risk of waitForCondition exceeding its 5sec default timeout, but... we should be okay.....
20:28ursulai guess we&#39;ll find out
20:29ursulai pushed the changes, do you want to sanity check before i attempt to land again?
20:31nanjursula: Mardak so you guys are playing around the waitForCondition? same here ;)
20:33nanji&#39;ve tried to put the delay behind the pref, and set it to 0 in the tests, it still gaves me intermittent failures
20:35Mardakursula: oh ha ha. i was running into waitForCondition nearly timing out doing 48 or 49 checks (it does up to 50 checks 100ms apart)
20:35Mardaki just realized i set the timer delay to 5 seconds ;p
20:35ursulatalk about cutting it close ;)
20:40streaminatordmose: Hey! Someone requested a review:
20:43Mardakursula: ha someone noticed! :p
20:44ursulawe almost got away
20:44streaminatork88hudson: Hey, your patch was approved!
20:45ursulaMardak: what&#39;s the preferred look for a multi line object after the || ?
20:45Mardaki would just do..
20:48dmosek88hudson: so, doing a bit more testing, it feels like the first start with about:home looks kinda strange because it&#39;s moving aroud
20:49k88hudsondmose: how so?
20:49k88hudsonlike how does it move
20:49dmosek88hudson: everything renders, and then it moves upwards by, say, 50-80px
20:50k88hudsonhm interesting
20:50dmosethis is on a macbook pro
20:50dmoseso reasonably quick machine
20:50k88hudsoncan you tell what&#39;s causing the move?
20:50k88hudsonis it the migration thing?
20:50dmosenot specifically
20:51dmosek88hudson: what would be the easiest way to tell?
20:51ursulaMardak: LOL almost just landed it with a delay of 8 seconds
20:51dmoseursula: ah, what could possibly go wrong?
20:51k88hudsondmose:is your migration message on
20:51dmosei&#39;m not seeing it
20:51ursulai&#39;m just being generous
20:51k88hudsonursula: eh 1/8 pretty close
20:51k88hudsondmose: can you see any obvious differences in the dom
20:52dmosek88hudson: between before and after the shift?
20:52dmosek88hudson: how would i make it stop pre-shift so that i can actually look at it?
20:53k88hudsondmose: add a setTimeout in activity-stream.jsx around the React.render
20:53k88hudsonfor like 2 seconds
20:56dmoseweirdly, when i call render from a 5s setTimeout, I don&#39;t see the shift
20:56dmosei wonder if there&#39;s some weird reflow interaction going on
20:57k88hudsoncould be yeah
20:58dmoseback in 5m; will get you a video
20:59k88hudsonyou could quicktime it
20:59k88hudsonalright cool
21:01k88hudsonMardak: dmose: do we want to do an export tonight
21:02Mardakespecially if we get highlights ;)
21:10dmosek88hudson: Mardak:
21:12dmosek88hudson: calling setTimeout(doRender, 100) has the jumping, using 200 does not
21:14k88hudsonlooking at it frame by frame, it looks like it&#39;s not jumping
21:14k88hudsonalthough the shadows make it sort of look that way
21:15k88hudsondmose: want to hop on vidyo real quick
21:16dmosek88hudson: your room?
22:09streaminatorursula: Hey! Someone requested a review:
22:18streaminatorMardak: Hey, your patch was approved!
22:19Mardakhey streaminator you&#39;re too slow
22:19Mardak(seems like github was backed up :p)
22:23k88hudsondmose: ok back
22:23k88hudsondmose: any luck?
22:23dmosek88hudson: ok, my room again?
22:23k88hudsondmose: sure
22:31streaminatorMardak: Hey! Someone requested a review:
22:47streaminatorr1cky: Hey, your patch was approved!
22:47streaminatorr1cky: Hey, your patch was approved!
22:48Mardakdmose, k88hudson: did i miss the followup fun?
22:48dmoseyes, buyt join in now
22:48k88hudsonMardak: i think we found it
22:49Mardakkate&#39;s room?
22:50dmoseMardak: mine
22:51dmoseMardak: er wait
22:51dmoseMardak: w&#39;ere in skype
22:51Mardakthat&#39;s ok. i&#39;ll try to track down a regression
22:51dmosebecause it turns out that Skype screen-sharing is amillion times less laggy
22:51dmosethan vidyo
23:07streaminatork88hudson: Hey, your reviewer requested some changes:
23:18Mardakk88hudson: shall i merge highlights and do an export?
23:24dmosei think she&#39;s AFK for a while
23:25dmosebut i don&#39;t see why not
23:27* dmose AFKs
23:34k88hudsonMardak: ^
23:34k88hudsonI just fixed up the last thing
23:34k88hudsonso if it&#39;s all good, I&#39;d love to be able to get it in
23:35k88hudsonI wonder if there will be any merge conflicts
23:37k88hudsonphew doesn&#39;t look like it
23:46Mardakk88hudson: did you want to squash some of those commits together or just merge?
23:49k88hudsonMardak: yeah I should
23:50Mardakk88hudson: so in the non-preloaded case, what happens when there&#39;s no strings? it&#39;ll end up rendering lpaceholder ids?
23:59k88hudsonYeah, I&#39;m gonna file a follow-up for that
8 Sep 2017
No messages
Last message: 12 days and 15 hours ago