mozilla :: #activity-stream

9 Aug 2017
14:48k88hudsonI'm back from PTO today btw if anyone was looking for me
15:16jkerimk88hudson ramen?
16:24Mardakr1cky: hand rolled screenshots? are we packaging screenshots instead of icons as part of tippytop?
16:26r1ckyMardak: yeah, my plan is to create a special manifest and reuse tippy top provider to loaf them in
16:26Mardakis that approach ok with legal?
16:28r1ckyMardak: yeah I guess screenshots are ok. bryanbell ^^
16:38dmoseMardak: i think the only sane thing to do here is go ahead with browser id tagging
16:38dmoseMardak: everything else sounds super fragile, if even possible
16:40dmosemconley: i've got an idea i'd like to run by you on vidyo, when you've got a few mins
16:41ursulaMardak: if i'm not in a location that gets pocket, but toggle browser.newtabpage.activity-stream.feeds.section.topstories to true from about:config, i'll get pocket recommendations right?
16:41ursulathe api key and all the other prefs get set automatically if the feed is on right?
16:42Mardakursula: yup
16:42ursulasweet, thanks
16:42jkerimk88hudson: if you get a chance pls r
16:42k88hudsonjkerim: sure
16:42jkerimtyty :3
16:42Mardakdmose: there will probably still need to be some in-page detection for some types of loads at least in the mid-term
16:43dmoseMardak: in addition to id tagging?
16:43Mardakdmose: short term i could see us instead of notifying browser-open-newtab-start, we would pass an arg of maybe { newTabID: } into openUILinkIn
16:44Mardakdmose: e.g., if the user navigates to about:newtab by typing it in
16:45dmoseMardak: and why would that also require in-page detection?
16:45Mardakhow are you going to set the triggering action and newTabID for that?
16:46dmosewe'd have to add code into the appropriate code path
16:46dmoseie somewhere in the code that handles that typed-in text, or further down the stack
16:46Mardakand are we doing that now/mid-term?
16:47dmosewe'd do it as we add load_trigger_types
16:47dmoseright now, we only support one of those
16:47dmoseand we know where to put it there
16:47dmoseas we add each new one, we put it in the appropriate spot
16:48dmoseso more or less now
16:48dmosebut we only fix the current spot for 56
16:48dmoseassuming we can even get that uplifted
16:48Mardakwell for 56 it needs to have non-a-s code changes
16:48dmoseright, those would need to be uplifted too
16:48dmosebut they should be pretty small
16:49dmoseand very unlikely to cause other problems
16:50Mardakchanging how firefox opens tabs is relatively intrusive to common firefox usage behavior. who knows what will happen..
16:50dmoseMardak: it's adding an optional parameter that will be ignored by all existing code
16:50Mardaksure... but it's an existing object that other callers use
16:50dmoseit's not obvious to me how that could break things.
16:50dmoseright, and...?
16:51Mardaki'm just saying the risk is non-0 :p it should be safe i agree
16:51Mardakbut who knows! ;)
16:51dmoseit's awfully close to 0, i'd guess
16:52Mardak"deleting this comment is no risk!" -> crashes builds
16:52dmoseas i said, i'm not asserting that it's actually 0
16:52dmose but that it's within epsilon
16:52dmoseanyway, ok, suppose we need to do something else for 56
16:53dmoseas you pointed out in the bugk, it's unclear how to do the matching up in a clean way since the other triggers aren't yet implemented
16:54dmoseMardak: is your though that we just use a bunch of in-page heuristics for 56 ?
16:54dmose(along with that preloaded detection)
16:54Mardakthe simpler fix i could see for 56..
16:54Mardakin the case of opening 3 tabs quickly: even if we wrongly pair only the most recent mark to the first visibility. this means 2 tabs will be unexpected
16:55Mardakis that 3rd-mark-with-1st-visibility all that bad? and is it bad to have the 2 unexpected?
16:55dmoseMardak: and just live with them being unexpected?
16:55dmoseliving with them being unexpected is probably fine
16:55Mardakmaybe another way to ask. is this better than having the mark associated to preloaded?
16:56dmosemistakes are only a problem if there are lots of them
16:56dmosei mean, i'd guess that all these "open multiple new tabs very quickly" scenarios are going to be lost in the noise
16:57Mardaka minorly different approach that is even simpler in logic: don't consume the mark. just take the most recent mark and associate with tab on visibility
16:57Mardakin this situation, all tab open marks were pretty close in time anyway....
16:58Mardakit will incorrectly associate a reloaded about:newtab page to the previous time it was actually opened/marked
16:58Mardakbut that's no different from what we have now
16:58dmosereloading seems also fairly rare
16:58dmosecompared to opening a single new tab, anyway
16:59Mardakdmose: so then just move setLoadInfo to the visibility SESSION_PERF_DATA and be done?
17:00dmoseMardak: for 56, that sounds reasonable at first blush
17:00dmoseoh, hey, standup time
17:03dmoser1cky: standup?
17:23Mardakr1cky: release drivers would prefer things get tested on nightly first, so just make a PR against master and we'll let it bake on nightly for a few days before requesting uplift
17:29Mardakdmose: we want both setLoadInfo-on-visibility as well as raf-setTimeout?
17:31dmoseMardak: ideally, yeah, but we could live without raf-setTimeout if we had to
17:31dmoseMardak: i'm assuming you're asking about what we want on 56
17:31Mardakdmose: including the multiple-topsites events?
17:32dmoseMardak: i think we want multiple-topsites events, but we could probably live without that too
17:33dmosealthough, actually
17:33dmoseif we take all three, then the graphs will be closer to what we'll have in 57
17:33dmoseotherwise, we'll need to create separate per-version graphs
17:33Mardakdmose: sure just update the PR with setTimeout instead of Promise
17:33Mardakk88hudson,dmose: these are things to be uplifted:"Release %2F Blocking" label%3AFx56
17:35dmoseMardak: i need to afk for about an hour, then i'll dig in to both of mine
17:41k88hudsonMardak: that looks good to me
17:57Mardakk88hudson: ok we'll need this backport for the upcoming export
17:58streaminatork88hudson: Hey! Someone requested a review:
18:03r1ckydmose: gah! forgot and went to lunch
18:04r1ckyMardak: sounds good re:nightly
18:06streaminatorMardak: Hey, your patch was approved!
18:09streaminatork88hudson: Hey! Someone requested a review:
18:54dmoser1cky: heh, no worries
19:11mconleydmose: heyo
19:11mconleywhat's up?
19:12dmosemconley: so, i want to chat a bit about perceived performance and how to track it around new tab time
19:12dmosemconley: up for jumping on vidyo for a few mins?
19:13mconleydmose: no guarantees I'll be able to provide much value, but sure
19:13mconleydmose: I'll drop into the Activity Stream room?
19:13dmoselet's do my room,
19:13dmosein case someone else has that scheduled
19:13dmoseMardak: ^
19:55Mardakdmose: i believe passing a promise as the first arg to notifyObservers just works because it's just an "object"
19:56Mardakvs passing around a function to the first arg is a bit trickier
19:59dmosehuh, interesting
20:01Mardakdmose: here's sample notifying promise: Services.obs.addObserver(promise => promise.then(console.log), "ed-foo"); Services.obs.notifyObservers(new Promise(resolve => setTimeout(resolve, 1000, "hello world")), "ed-foo")
20:03dmoseMardak: i'm assuming we'll just have the promise pass back the browser itself, correct?
20:04Mardakyeah resolver(b)
20:05dmosegood times!
20:16Mardakk88hudson: i believe in this current case, beta and m-c are both the same code-wise for activity stream. in the next export to m-c, i'll export the firefox-56 branch and follow up on that with an actual master branch export. typically a m-c bug's patch is uplifted to beta as opposed to a beta-specific patch
20:19Mardakr1cky: how soon is soon for next icons?
20:19Mardaker. screenshots
20:20r1ckyMardak: end of week to have them all
20:21Mardakok hrm. to have just one uplift-to-56 request. ideally we have everything in one patch
20:30r1ckyMardak: ok. we can hold off then
20:30r1ckymakes it easier
20:31Mardakbut if we don't have the firefox-56 branch export patch ready until end of this week, then we'll also hold off on the master branch export patch
20:32Mardakr1cky: instead of guarding, does leaving that code untouched just work?
20:33r1ckyMardak: yeah, that's what I'm testing now. i think it will be fine if it's undefined
21:04streaminatorrlr: Hey! Someone requested a review:
21:07Mardakdmose: what is " this is presumably, a fairly rare case that will get lost in the noise.
21:07Mardakthe "this"
21:08dmoseMardak: that's unclear; i'll fix
21:09Mardakdmose: but before that, what were you trying to say?
21:12streaminatorMardak: Hey, your patch was approved!
21:12dmoseMardak: how's this:
21:12Mardakr1cky: this "copr" git alias is working quite nice :)
21:13r1ckyMardak: yep. removes that mental barrier to trying something locally
21:13dmoser1cky: copr?
21:14dmoseoh, checkout pr
21:14Mardakour check-out-pull-request git alias
21:14Mardakdmose: looks good. the original comment seemed wrong if "this" meant rendering when hidden as preloaded hidden rendering is actually very common
21:14* dmose commits and pushes
21:15dmoseMardak: done
21:18streaminatordmose: Hey, your patch was approved!
10 Aug 2017
No messages
Last message: 14 days ago