mozilla :: #activity-stream

8 Sep 2017
00:08Mardakk88hudson: you'll squash and merge when you're ready and i'll do an export?
00:18Mardaklooks like we might hvae a test failure
00:21Mardakah. seems like andreio might already have a fix for it
00:23k88hudsonah darn
00:24k88hudsonis that with aboutHome true?
00:24Mardaki suppose i can just take his try patch and land it
00:24Mardakyeah with abouthome true. intermittent on win32 debug
00:27Mardakk88hudson: well if it's already intermittent, hopefully we don't cause it to perma-fail and the export should stick
00:29k88hudsonok sounds good
00:29k88hudsonMardak: ok all rebased
00:29Mardakk88hudson: i can export.. or i suppose you could export to Bug 1396609 - Add Highlights, prerendering capability and bug fixes to Activity Stream
00:29Mardakand i'll review and land..
00:30k88hudsonMardak: sure
00:30k88hudsonMardak: mind r+ing
00:31streaminator1k88hudson: Hey, your reviewer requested some changes:
00:34Mardakk88hudson: uho. looks like first load is broken
00:34k88hudsonMard :'(
00:34Mardak./mach run about:newtab or aboutHome.enabled = true
00:34Mardakbut otherwise, it works!
00:35Mardakso.... ship it!?
00:36streaminator1k88hudson: Hey, your patch was approved!
00:37* k88hudson tries
00:37Mardak./mach run --setpref browser.newtabpage.activity-stream.aboutHome.enabled=true
00:37k88hudsonoh cool
00:37k88hudsonthat's useful
00:38Mardakoh well if aboutHome.enabled is broken, it's quite likely it'll fail some mochitest
00:38Mardakhrmmmm.. doing quick local test.
00:39k88hudsondamn you're right
00:39Mardak66 INFO TEST-UNEXPECTED-FAIL | browser/extensions/activity-stream/test/functional/mochitest/browser_as_load_location.js | Got <body class=&#39;activity-stream&#39;> Element -
00:40Mardakchecking without prerendering PR
00:41Mardakk88hudson: :( at least as_load_location passes without prerendering PR
00:41k88hudsonMardak: looks like NEW_TAB_STATE_REQUEST gets fired before AS is ready basically
00:42k88hudsonwhich is definitely possible
00:43Mardakah indeed. and it worked before because ASMC simulates onNewTabLoad when inheriting
00:44Mardakk88hudson: have ASMC fake a NEW_TAB_STATE_REQUEST ?
00:45k88hudsonMardak: that could cause it to happen too early though
00:45k88hudsonwe could... poll :S
00:47Mardakk88hudson: i suppose the more correct thing is have ASMC dispatch to each just-inherited port that it&#39;s ready
00:47k88hudsonMardak: like a handshake-type thing
00:47Mardakthat kinda happens with INIT broadcasting
00:48Mardakso if content receives INIT, it knows it initially loaded before ASMC was ready
00:48Mardakand re-request STATE_REQUEST !
00:48k88hudsonthat&#39;s true
00:48k88hudsonnice that will work
00:48k88hudsonweirdly i didn&#39;t see INIT though in the logs
00:49k88hudsondo we not broadcast it now i guess?
00:49Mardakshould still have ac.BroadcastToContent({ type: at.INIT,
00:49k88hudsonyeah it does
00:50k88hudsonthat&#39;s odd
00:51Mardakk88hudson: i see INIT
00:52k88hudsonah ok
00:52k88hudsonyeah i see it
00:57Mardakk88hudson: booo as_load_location still fails i think
00:57k88hudsonMardak: ah you know what
00:57k88hudsoni think pocket is turned off in the tests
00:58k88hudsonso that would make it use the unprerendered version
00:58Mardakbut they should all be using unprerendered as the newTabService patch isn&#39;t there
00:59Mardakthat&#39;s why NEW_TAB_STATE_REQUEST is happening on page load to begin with
01:03Mardakk88hudson: OH WAIt!
01:03Mardakas_load_location is looking for body.activity-stream
01:03Mardakrestore that classname!
01:03Mardak..testing locally
01:04Mardakk88hudson: TEST PASS!~
01:04k88hudsonAW YEAH
01:04k88hudsonok i&#39; mdoign this check for INIT thing right now
01:05k88hudsonwhile instructing jon how to cook dinner
01:05Mardak;) i hand patched bundle.js :p
01:06* tspurway rejoices a little
01:06Mardakchecking if other tests fail..
01:07Mardakok _UsageTelemetry_content_aboutHome.js passes locally
01:20Mardaki pushed to try to check other potential test failures but included body.a-s fix and INIT change
01:23streaminator1Mardak: Hey! Someone requested a review:
01:41Mardakr1cky: probably just land the PR as is for now and see if design has more opinions
01:42r1ckyMardak: sounds good to me
01:42streaminator1r1cky: Hey, your patch was approved!
01:42k88hudsondarn, i&#39;m seeing some other problems with the strings not getting updated in manual migration for example
01:43k88hudsonMardak: it&#39;s getting close to 7, maybe we should hold off anotehr day on the prerendering and just do the highlights
01:46Mardakk88hudson: ok. looks like highlights might be regressing FAIL | browser/base/content/test/performance/browser_startup.js | resource://gre/modules/Bookmarks.jsm is not allowed before handling user events
01:46k88hudsonhm darn
01:46Mardakk88hudson: also simple fix but 18:27:54 INFO - TEST-UNEXPECTED-FAIL | browser/base/content/test/static/browser_all_files_referenced.js | unreferenced file: resource://activity-stream/data/content/activity-stream-prerendered.html -
01:47k88hudsonoh yeah
01:49k88hudsonMardak: ok this should fix the rehydration after init stuff, i need to do tests through
01:53streaminator1Mardak: Hey! Someone requested a review:
01:58streaminator1r1cky: Hey, your reviewer requested some changes:
01:58streaminator1r1cky: Hey, your reviewer requested some changes:
02:02k88hudsonMardak: should i still do the export with just highlights?
02:02k88hudsonor is that test failure a no go
02:03Mardakoh oh maybe i have a fix. trying
02:04Mardakah no. i need to figure out what&#39;s loading places sooner than we want
02:04Mardakif i figure it out, i guess i&#39;ll export and r?dmose
02:05k88hudsonok sounds good
02:33k88hudsonMardak: ok has tests
02:33k88hudsonI added the classname back in too
03:17k88hudsonalright it&#39;s workiiingg
03:17k88hudsoni think
05:10Mardakdmose: around for review? ;)
05:10dmoseMardak: rs=, anyway
05:10dmoseMardak: any thoughts on that talos comment?
05:11Mardakhow do we think talos will change? my previous runs with using -prerendered.html didn&#39;t seem to have much impact
05:11dmosewoohoo, PerformanceObserver is now going to rid the trains
05:12Mardakand by default without the newTabService change, it&#39;ll just use the base .html
05:12dmoseMardak: the race fixes cause the time between the pre-rendering and the attaached rendering to be longer
05:12dmoseMardak: is the idea to turn on about:home for this export also?
05:12Mardaki dont think so
05:13dmoseok, in that case, it&#39;s not such a big deal
05:13Mardakk88 filed a bug to turn on newTabService to use prerendered
05:13dmoseah, right, and that&#39;s not yet landed
05:13dmoseso sure, exporting now seems quite reasonable, in light of that
05:17Mardakdmose: we do need to fix the unreferenced prerender.html test failure if we do land prerendering changes
05:17Mardakmight just be adding a whitelist
05:18dmosewell, it is adding the whitelist
05:18dmosebut i assume that needs to be a separate bugzilla landing of some sort
05:19dmosethuogh I suppose it could be in the same bug
05:20dmoseMardak: we could just push this stuff to master tonight, fix that locally with a patch, let pine run, and then export tomorrow
05:20dmosebut i understand the desired to land sooner :-)
05:24dmoseMardak: i need to crash very soon, so if you do want to get a review tonight, now would be the time to request
05:26Mardakdmose: i guess tomorrow morning will be fine
05:26Mardakmost likely won&#39;t make it in to tomorrow&#39;s nightly anyway
05:27dmoseMardak: sounds good
05:47streaminatorr1cky: Hey! Someone requested a review:
09:58streaminatorMardak: Hey, your patch was approved!
09:58streaminatorMardak: Hey, your patch was approved!
13:58ahillierMardak, r1cky: Sorry about that, I had thought that the `BroadcastToContent` at the end of the `INIT` refresh would have caught any tabs that loaded in the meantime, but apparently not.
14:17streaminatorandreio: Hey, your patch was approved!
14:25ursulaahillier: can i close this:
14:25ursulasince the patch landed in mc?
14:25ahillierursula: Oh yeah that&#39;s a good point, go for it
14:26ursulaahillier: and now this: is unblocked?
14:26ahillierursula: yep looks like it
14:36Mardakk88hudson: should we just export highlights now and do a separate export for prerendering later today?
14:36k88hudsonMardak: yeah go for it
14:36k88hudsonI&#39;m just debugging that test failure right now
14:37streaminatorr1cky: Hey, your patch was approved!
14:49Mardakursula: compared to last time, this one /only/ has 1k lines changed ;)
14:53ursulapsh easy peasy
15:00streaminatork88hudson: Hey! Someone requested a review:
15:09ursulaMardak: wait so this patch does include pre-rendering? i don&#39;t see it
15:09Mardakursula: no prerendering just highlights
15:09ursulai&#39;m just reading your comment where it says it includes unmerged pr
15:10Mardakrevision 1 had prerendering
15:10ursulai see
15:10Mardaksorry i updated title and story but didn&#39;t comment
15:10ursuladidn&#39;t see the title change after the comment
15:10ursulano worries
15:10ursulathat&#39;s my bad
15:14k88hudsonMardak: lol the race condition is happening because search is mounting *too* fast, lol
15:14k88hudsonso the ready event is getting sent before the test adds the listener
15:14Mardakha ha
15:14Mardaki guess we can do something similar to andreio&#39;s fix
15:14MardakwaitForCondition on globalSearch.searchENgine or whatever that property is
15:16dmoseno, just add some busy waiting to componentWillMount
15:16Mardakursula: can&#39;t forget about the pref animation fix! &quot;clicks this animation every day because it&#39;s the sexiest in the product&quot; &quot;haha .. must make prefs panel sexy again&quot;
15:16dmoseslow things down
15:16k88hudsonMardak: andreio: you mean instead of listening to the ContentSearchService event
15:17Mardaknod. the test is awaiting that event. instead await for ContentTask waitForCondition
15:17ursula activity stream: making animations sexy again
15:18k88hudsoncool, that sounds good
15:18andreioyeah what happens is that after the Search component loads it triggers events to load the search provider (in the constructor) but who knows when the search provider is ready.
15:19andreioit will initialize defaultEngine when ready
15:38Mardakk88hudson: hrmmmmm... i locally just did some screen recording measurements and there&#39;s no difference in hero element timing if we say it&#39;s when the placeholder text appears. makes sense as strings aren&#39;t loading any faster/slower than before
15:39Mardakon my debug build, strings appear 16 frames (30fps) after browser first paint with prerenddering and not
15:40k88hudsonyeah for placeholder text, obviously not -- we were talking about either putting that one string in or moving the test to check the input itself
15:40k88hudsonmaybe try it with putting the placeholder string in the default strings?
15:40Mardaknod. if it&#39;s for the input, prerendering makes it show up even before onboarding in 8-10 frames
15:41Mardakwell it should be the same result as that 8-10 frame to first paint that includes search box
15:41k88hudsonok looks like checking gContentSearchController fixes the test
15:42Mardakk88hudson: i guess make it a moz-central-patches file that needs to be also applied on export (also for patching the unreferenced file)
15:42k88hudsonMardak: should i include that as a patch in in mozilla-central-patches/ with the PR or file a separate issue do you think
15:42Mardaknod nod
15:42k88hudsonok cool
15:44k88hudsonoh last thing
15:44k88hudsonshould i include highlights?
15:45k88hudsoni could do that as a follow-up i guess
15:58dmosedo it as a followup
15:58dmoselet&#39;s get this thing landed
16:45k88hudsonyeah i agree
16:49tspurwayso for those who are in a vidyo room wondering about OKRs, I cancelled that meeting
16:49tspurwayi wanted to survey if yall wanted to have a 1pm EST meeting to quickly triage inbound tickets and re-examine our P1s
16:49tspurwayor if mondays triage is sufficient
16:50streaminatorahillier: Hey, your reviewer requested some changes:
16:50streaminatorahillier: Hey, your reviewer requested some changes:
16:50tspurwaymardak, dmose, ursula, k88hudson, nanj, r1cky, ahillier, andreio
16:51dmosetspurway: the sooner the better, i think
16:51dmoseand it&#39;ll shave a bunch of time off of monday&#39;s triage
16:52dmoseso it won&#39;t be too much extra burden
16:52dmosebut, that&#39;s really assuming that it won&#39;t unduly delay prerender landing
16:55k88hudsondmose: Mardak: do yall have like 15 mins to discuss the prerendering/hero element test
16:56dmosek88hudson: what do you think re triage (see above) vs this?
16:56k88hudsondmose: i&#39;m happy to do both
16:57tspurwayok lets do it
17:02tspurwaymardak, ursula we are in Activity Stream vidyo
17:29streaminatorandreio: Hey, your patch was approved!
18:23streaminatorr1cky: Hey! Someone requested a review:
18:24streaminatorursula: Hey, your patch was approved!
18:38k88hudsondmose: ok, rebased
18:42Mardakk88hudson: you wanted to chat about the numbers i ran for prerendered hero ? or did that already happen
18:43k88hudsonMardak: oh yeah, we chatted briefly about what we should do to resolve the issue with it looking for the placeholder text
18:43Mardakoh yeah and do it for english
18:43Mardakoh right and i could look into the thingy
18:44dmosek88hudson: so the migration snippet is making things shift down after prer4nder
18:44Mardakthe tricky part there is we&#39;re now using (i forget actual name) content languages vs app locale
18:44dmosebut i guess that can&#39;t be helped much
18:44Mardakand uses app locale
18:46streaminatork88hudson: Hey, your patch was approved!
18:47dmosek88hudson: r+ in the patch, i&#39;m heading to an appt, will be available for review bits later this afternoon, otherwise will be around more on sunday
18:47k88hudsondmose: you&#39;re the best!!! thank you!
18:47k88hudsonand Mardak :D
18:48k88hudsonMardak: I was thinking eventually it would be great to just go with hard-coding the locale in and be done with it, remove strings from redux too
18:49k88hudsonbecause it would simplify things a lot
18:49k88hudsonwhat&#39;s the difference between content languages / app locale
18:49Mardakuser can say i&#39;d prefer german then french then english for content languages
18:49k88hudsonah right
18:49Mardakapp locale is the one firefox built with
18:50k88hudsonwhat does the rest of firefox UI use
18:50Mardakfirefox UI uses app locale. although sounds like localization wants to move away from that
18:51k88hudsonyeah, that makes sense
18:51k88hudsona list of preferred langauges seems like a way better experience
18:56ursulaMardak: turns out i was wrong.... is a 14 line change
18:58k88hudsonbut if would definitely be easier to just suppport one
18:58Mardaki believe review time is superlinear to size of patch... let&#39;s say quadratic. i&#39;ll need 196x longer to review! :p
18:59Mardakk88hudson: for jar stuff.. it might depend on putting things in content -> chrome://activity-stream/content/..
18:59k88hudsonMardak: oh damn
19:00Mardaksearching through old m-c history..
19:03Mardakloop ended up with this
19:07Mardakseems like i should be able to have a huge AB_CD if else if else if else if to package a specific file...
19:07Mardakif anyone else has more experience ;)
19:09Mardakoh or alternatively, make sure we have a file for all possible AB_CD (even if it ends up being english)
19:22Mardakursula: 1 line change.. on wrong line! ;) oops :p
19:23ursulaMardak: i think you may be right about not even needing it :o
19:23ursulalet me look at the api call now
19:24Mardakdmose: mmmmmmmmm do you know?
19:24Mardakidl says 3rd parameter is optional
19:24Mardakdoes xpcom make it 0?
19:25ursulai have no idea
19:25ursulai guess.. apparently....
19:25ursulaprobably better to be explicit though
19:36k88hudsonMardak: wow look at that
19:38k88hudsonthat&#39;s exciting lol
19:49Mardakk88hudson: sounds like we&#39;ll want to turn on aboutHome.enabled with prerender landing
19:51Mardakthere&#39;s 3 possible commits: export, newTabService prerender, aboutHome enable
19:53k88hudsonMardak: ah yes, true
19:53k88hudsondo you want me to put another patch up?
19:54Mardakif we suspect any of them individually might get backed out but want the others to stick, they should be kept separate
19:56streaminatork88hudson: Hey! Someone requested a review:
19:59ursulaMardak: updated the patch
20:02ursulathanks :D
20:10Mardakk88hudson: which patch are you referring to? export prerender (and any other a-s master changes?)
20:11k88hudsonMardak: i mean to turn on aboutHome
20:11k88hudsoni guess you can just add that in
20:12Mardakoh wait. we still need to fix tests to turn on aboutHome
20:14k88hudsonis the fix just to skip it?
20:15streaminatorahillier: Hey, your patch was approved!
20:16ahillierHeads up - testpilot code will get removed on next pull
20:18ahillier-> `npm install && git clean -d -f` <-
20:59Mardakk88hudson: is browser_google_behavior.js passing for you?
21:28dmoseMardak: ursula: re the optional param, it looks like the C++ code assumes zero means optional, further down the call chain;
21:43dmoseMardak: k88hudson is it wise to turn on aboutHome in the same export as the prerender?
21:43dmosei&#39;m concerned that it&#39;ll make issues more complicated to track down
22:49Mardakdmose: do we actually want the newTabService patch in m-c-patches? seems like it should just be its own bugzilla patch unless we want pine testing with prerendered?
23:46dmoseMardak: hmmm
23:47dmoseMardak: I suppose it does leave us temporarily testing against a future state, not the current one
23:47dmoseMardak: I guess i assumed that it should be it&#39;s own bugzilla patch as well and would land quickly and allow us to remove it from m-c-patches
23:49dmoseMardak: is our current plan to export next before we land that patch?
23:49dmose(once it&#39;s created)
9 Sep 2017
No messages
Last message: 13 days and 1 hour ago