mozilla :: #fx-team

9 Aug 2017
00:04adwah my system clock was wrong, causing the ssl connection to fail
08:28nhnt11https://twitter.com/dpnation/status/895110349497741313 :(
09:40Standard8where do I look for bugs on the new photon hamburger menu? Specifically the Library / Bookmarks section
09:50Standard8well, what I thought was a bug isnt, so thats good :-)
14:11mconleyperry: pong
14:17florianTIL: nsITimer has a TYPE_REPEATING_SLACK_LOW_PRIORITY type that is even lower priority than idle callbacks. http://searchfox.org/mozilla-central/rev/0f16d437cce97733c6678d29982a6bcad49f817b/xpcom/threads/nsITimer.idl#119
14:24Sylvestredao, r? on bug 1388628 so that I can land that in m-r ?
14:24firebothttps://bugzil.la/1388628 NEW, nobody@mozilla.org Tabs are all restored as blank frequently after restart of applying Firefox 55 update. Windows 10 (x
14:39felipemikedeboer: ping
14:40mikedeboerfelipe: pong
14:40mikedeboerfelipe: I saw your comment - let's move on without PromiseUtils
14:41felipemikedeboer: ah that was a comment from florian, but yeah that was the reason I didn't use it
14:41felipemikedeboer: I have a question about this _browserSetState business
14:41mikedeboer(as an aside: it'd be cool to have these tidbits of information shared on firefox-dev or fx-team...)
14:41felipemikedeboer: can you explain to me what it is, so that I can add a proper comment there
14:42felipeI just followed the code to do the right thing, but I don't know why there are these two different notifications
14:46mikedeboerfelipe: hmmm, that might be legacy/ old mistake. When I think about it now, I'd expect the NOTIFY_WINDOWS_RESTORED to always be broadcasted, regardless whether we're restoring from `setBrowserState()` (usually done by tests only) or using a file read (the regular method by users)
14:49felipemikedeboer: hmm, interesting.. maybe there are tests which didn't want to trigger more than once all the observers of NOTIFY_WINDOWS_RESTORED
14:49mikedeboerfelipe: that's what I'm betting on as well
14:50felipemikedeboer: ok cool.. i'd rather not modify this on this patch, so I'll leave the behavior unchanged (using the style you proposed) and add a comment there
14:51mikedeboerfelipe: cool, sgtm!
15:43zombiemy browser console is missing a command line, in a fresh profile.. seems on purpose?
15:44zombieah, nvm me
15:51daoSylvestre: I'm trying to verify that the backout fixes the problem on release. unfortunately artifact builds seem to be broken there so I'm doing a full build
16:05perrymconley: I can still do bug 1365305 (I had a fix in mind), or do you want someone who can reproduce the reflow?
16:05firebothttps://bugzil.la/1365305 REOPENED, jiangperry@gmail.com 0.86ms uninterruptible reflow at _positionPinnedTabs@chrome://browser/content/tabbrowser.xml:6147:33
16:10mconleyperry: yeah, I think it's still worth giving it a shot
17:04Gijsursula: did you intend to revert Marco's changes to https://bugzilla.mozilla.org/show_bug.cgi?id=1388528 ?
17:04firebotBug 1388528 ASSIGNED, adw@mozilla.com The Page action menu should remain visible on the about:newtab and about:home Pages.
17:05ursulaGijs: oh woah
17:05ursulamid air collision?
17:05ursulai added someone to the cc list it must have screwed something up
17:06ursulain short, no i did not
17:06Gijsursula: I figured. I'll fix it up, no worries
17:06ursulaGijs: thanks, sorry bout that
17:14ursulaGijs: did you manage to get pocket stories with that pref?
17:33Gijsursula: yup, still not sure why it's broken, there's 0 errors in the browser console or anything like that.
17:33Gijsadw: did you figure this out already?
17:33adwGijs: not yet, planning on looking at it today
17:34adwi also noticed there's nothing in the error console
17:34adwone thing is that it's probably not looking for the page action button at all
17:35adwi.e., if the pocket button is hidden in the urlbar, it should use the page action button
17:35adwof course if it's not hidden, then it should use that
17:36adwi wonder what it's doing now, when it expects the pocket button to be in the toolbar, but when the user has moved it out of the toolbar
17:37adwmaybe why there are no errors now is that it checks whether it could find the expected toolbar button and just silently fails if it can't
17:37adwanyhow, should be pretty simple to fix
17:39ursulaadw: Gijs i think it's hitting this: http://searchfox.org/mozilla-central/source/browser/extensions/pocket/content/Pocket.jsm#98 if it doesn't find the pocket button
17:39ursulaso it just bails out of everything
17:40adwmakes sense
17:40ursulabut adding the meatball menu back in about:newtab won't fix that either
17:41adwi think we should show the door hanger on the meatball if pocket is hidden
17:41adwis that what you mean?
17:41adwthat's what we do for the bookmark doorhanger
17:41Gijson about:newtab the meatball doesn't show either
17:41Gijsdoes this mean the menu items are also broken?
17:41adwright, that's part of what that bug is about
17:41Gijslike the context menu
17:42ursulai couldn't find any code that decides when to show the meatball menu on a page or not
17:42adwlooks like it
17:42GijsHm, looks like we don't show the pocket context menu anymore either :(
17:42adwursula: it's there somewhere, i forget where :-)
17:43Gijsadw: might be easiest to just take this opportunity to rip the pocket button itself out and then anchor on the (i) if the meat ball / pocket thing aren't around
17:43adwGijs: what do you mean by the pocket context menu?
17:43adwGijs: so -- not show the meatball on about:newtab after all?
17:44adwor as the last backstop, use the (i)?
17:44Gijsadw: there's a context menu item for pocket in the main context menu to save things to pocket, I think
17:44Gijsthe latter
17:44adwright, ok
17:44Gijswe can make the meatball vs. about:newtab decision separately
17:44GijsI dunno what shorlander wants to do there
17:44adwi thought you meant there was an entire context menu dedicate to pocket
17:44Gijsno no :)
17:44adwwell, bryan in that bug says to show the meatball button on newtab
17:44adw"The Page action menu should remain visible on the about:newtab and about:home Pages."
17:44GijsI mean this https://dxr.mozilla.org/mozilla-central/rev/a921bfb8a2cf3db4d9edebe9b35799a3f9d035da/browser/extensions/pocket/bootstrap.js#238-254
17:44Gijsyeeeeah
17:45GijsI think the urlbar-icons container being hidden there was a conscious decision way back when this stuff was in the url bar pre-australis
17:45Gijsbut I dunno
17:45Gijsshorlander has more context, I imagine
17:46GijsBut either way it seems like showing these icons by itself wouldn't fix the issue
17:46adwshowing the icons is just giving the panel somewhere to anchor, yeah
17:46adwneed to update the code that decides where to anchor
17:46adwa backstop of the (i) is reasonable regardless
17:48adwso maybe we should split that bug: (1) update the logic determining where to anchor, (2) show the meatball on about:newtab if that's actually what we want
17:49adwthe logic would prefer in order: the pocket urlbar button, the meatball, the (i)
17:52Gijsadw: feels like removing the toolbar button code will address (1), so if you want to split it you could try doing that bug first?
17:52Gijsor do you think that's going to be significantly more complicated?
17:53adwhmm, i guess not
17:53adwi think they're separable but i guess not
17:53adwi'll take that bug then and update the other bug with this convo
17:53felipemikedeboer: hey, I posted the updated patch to the bug, in case you want to take a look
18:00ursulaGijs, adw: can you hop on a call with bryan and I about this quickly?
18:01adwursula: yes, which room?
18:01ursulaActivityStream
18:01ursulaoh wait! adw
18:01ursulathere's another call going on there
18:02ursulaadw: join my room :)
18:10sfosterWhy do I remember there being a default/non-attention equivalent of var(-toolbarbutton-icon-fill-attention) ?
18:10sfosterUsed to provide the main fill/stroke color in the toolbar icons
18:10sfosterAll I can find now is http://searchfox.org/mozilla-central/source/browser/themes/shared/toolbarbutton-icons.inc.css#7
18:19sfosterdao: / johannh shouldnt we have a var for this color?
18:20daosfoster: https://bugzilla.mozilla.org/show_bug.cgi?id=1387723
18:20firebotBug 1387723 FIXED, dao+bmo@mozilla.com Remove --toolbarbutton-icon-fill and --toolbarbutton-icon-fill-inverted
18:21sfosterdao: ok that explains it at least I'm not going crazy
18:22daosfoster: so where and why do you want to set this?
18:22sfosterfor the toolbar button animations, some of those images use 2 colors - the dark grey and the attention blue.
18:23sfosterso in bug 1373341 I'm going through and fixing those to ensure themes do the right thing.
18:23firebothttps://bugzil.la/1373341 ASSIGNED, sfoster@mozilla.com Two different blue highlight states needed for light and dark theme (downloading, bookmarking)
18:23daocan't you just set the attention color when you need it and fall back to the default otherwise?
18:23ursulaadw: https://dxr.mozilla.org/mozilla-central/source/browser/extensions/activity-stream/lib/PlacesFeed.jsm#225
18:24sfosterdao: images like https://hg.mozilla.org/mozilla-central/raw-file/tip/browser/themes/shared/icons/library-bookmark-animation.svg have both.
18:25sfosterbut yes, I don't need to use css vars for this of course. But we'll see a few more fill: / stroke: properties with these colors in our css
18:26adwGijs: here looks like: https://dxr.mozilla.org/mozilla-central/source/toolkit/modules/NewTabUtils.jsm#1108
18:28daosfoster: you should be able to use context-fill in the svg for the default gray without setting it in your css. it should "just work". you can use context-stroke for the attention color
18:28sfosterdao: sound reasonable, I'll see how it plays out.
18:36adwGijs: i'll wontfix https://bugzilla.mozilla.org/show_bug.cgi?id=1388528, yeah?
18:36firebotBug 1388528 ASSIGNED, adw@mozilla.com The Page action menu should remain visible on the about:newtab and about:home Pages.
18:36Gijsadw: sgtm!
19:43ursulaadw, is this: https://dxr.mozilla.org/mozilla-central/source/browser/base/content/browser-places.js#1275 the api i should use to open the bookmark panel?
19:43adwursula: yes
19:44adwursula: caller example here: https://dxr.mozilla.org/mozilla-central/source/browser/base/content/browser-pageActions.js#643
19:45adwhmm although that method probably isn't what you want in this case since it takes an event
19:45ursulaerr also this is to bookmark the current page
19:46ursulawhat we want is to indicate that you've bookmarked a specific url
19:47adwah yeah true
19:48adwmaybe it would be ok to call like this directly: https://dxr.mozilla.org/mozilla-central/source/browser/base/content/browser-places.js#612
19:48adwhmm but that still wants a browser
19:48adwunderstandably
19:48ursulapassing in the browser is ok
19:49adwit looks like the browser of the page you want to bookmark though
19:49ursulaah i see
19:49adwthis might be the first time we have a ui that lets you bookmark pages that aren't actually loaded...
19:50ursularight, because in the case of these pocket links, they haven't even ever been visited
19:50adwyeah
19:50adwyou might have to add a new method on PlacesCommandHook to do this
19:51adwit looks like it might not be too hard to factor it out of bookmarkPage
19:51ursulawhat does: https://dxr.mozilla.org/mozilla-central/source/browser/base/content/browser-places.js#628 do?
19:51adwah, hmm
19:51adwi'm not too familiar with this
19:52adwPlacesUIUtils.showBookmarkDialog -- i wonder whether that's that kind of ugly dialog that opens sometimes instead of the doorhanger
19:52ursulai think it is
19:52ursulai'm assuming the nice one is the StarUI
19:52adwyes
19:53ursulai tried using the showBookmarkDialog out of curiosity and it the ugly one
19:53adwah, ok
19:53ursulabut could i change the bookmarkLink to just use the nice dialog?
19:54adwyeah, i think you're going to have to do something like that since what we want isn't there afaict
19:54adwalthough you might want to add a new method instead
19:54adwjust not to break longstanding behavior
19:54ursulayeah, there's a couple other places that looks like it uses bookmarkLink so i'll just add a new function
19:54ursulayeah
19:55adwok
19:56adwursula: maybe we should check with bryan first -- explain that we already have a dialog for cases where the page isn't open
19:57adwmight be better to stick with that in this case too?
19:57ursulayou mean the showBookmarkDialog?
19:57adwyes
19:58ursulasure, i'll swing it by bryan and see what he thinks
20:02Gijsit would be nice if we could replace the ugly modal thing with the doorhanger for editing bookmarks property
20:02Gijs*properties
20:02ursulaGijs: i'll give it my best shot ;)
20:03Gijs:D
20:06Gijswheee, there are now more structure bugs assigned to people than remaining mvp items
20:06Gijs(don't worry, there are like a gazillion reserve bugs left that we also need to fix some of...)
20:07Gijsjaws: speaking of which, is there something I can do to make review of my second patch in bug 1383009 easier? :)
20:07firebothttps://bugzil.la/1383009 ASSIGNED, gijskruitbosch+bugs@gmail.com Add flexible spaces around the URL and search bar by default
20:07Gijsjaws: like move it Mossop or something?
20:08* Gijs checks jaws' queue
20:08Gijsthat's a yes :)
20:08MossopDamn. I should have left the 23 patches in my queue yesterday
20:09jawsi'm in a meeting
20:09jawsbut i *think* my queue is empty
20:10Gijsjaws: 2 needinfo, 6 review?
20:10jawsoh i'll do those today then
20:11Gijsjaws: I can steal one back... on 57, for prefs changes, is just in-content-new enough?
20:13jawsGijs: not yet, maybe wait a week or two to know if any late-breaking issue with new prefs
20:13Gijshm
20:13Gijspdahiya: ^^ those prefs changes in 1387153, do they want to ship in 56?
20:13* Gijs suspects so...
20:21pdahiyagijs: yes, 1387153 is planned for 56
20:27Gijsok
20:45ursulaadw: i can actually get the nice ui to show up pretty easily without even creating a new function for it and just adding some conditional logic to the existing function. mind if i send a patch your way just for feedback on the approach?
20:46adwursula: sure
20:52ursulaadw thanks!
21:06adwursula: feedbacked
21:08ursulasweet, thanks :)
21:08jawsursula: hey, what's the bug for tracking the history view on the new tab page?
21:09ursulajaws: let me grab it
21:11ursulajaws: err, looks like we don't have one yet. you mean the "highlights" section right?
21:11jawsursula: i guess??? the part that was demoed heavily for the activity stream test pilot
21:12ursulayeah like those card looking things with a preview image and description
21:12jawsursula: the "Top Activity" in this screenshot, https://wiki.mozilla.org/images/thumb/7/71/Activity-stream-2016-03-11.png/800px-Activity-stream-2016-03-11.png
21:13ursulajaws: ahhh
21:13ursulaunfortunately we won't be porting that feature over :(
21:13jawsursula: i see, yeah, this: https://cdn.ghacks.net/wp-content/uploads/2016/05/activity-stream.jpg
21:13jawsoh?
21:13jawsursula: will the "Highlights" in that last screenshot get ported over?
21:14ursulayeah, we removed the entire timeline part, along with all the top activity stuff. the highlights part yes will get ported over
21:14jawsdid people not use it?
21:14ursulayeah it was the least popular feature activity stream. people didn't like that it exposed so much of their history right there for everyone to see
21:15ursulaour user research studies showed people weren't into it
21:15jawsokay, but if i must be frank, the name "activity stream" makes me think we would be showing some stream of activity?
21:15ursulayou're definitely not wrong there
21:15jawswithout that, the only difference between activity stream and the old "new tab page" is the "recommended by pocket" section
21:15jawswhich imo feels like just ads
21:16jawsthe "Hightlights" part will help
21:16ursulayeah, we're definitely getting a bunch of similar feedback on the recommended content. let me grab the latest specs it'll give you an idea of what they're thinking for highlights, give me a sec
21:16jawsthanks!
21:19ursulajaws: https://mozilla.invisionapp.com/d/main#/console/11068990/242532753/preview
21:20jawsursula: am i supposed to use my mozilla credentials there?
21:20ursulayeah your ldap should work
21:20jawssorry i guess i'm paranoid but this should be using 0auth, right?
21:20jawsalternatively, can we make these specs public?
21:20jawswhy do they need to be confidential?
21:21jawser, auth0
21:21ursulathey probably don't need to be. that's just the specs that i get from the designers. i'll see if they have another link to it
21:22jawsokay, or we can ask that it be made public
21:24ursulajaws: https://mozilla.invisionapp.com/share/HVCZ0YD5F#/screens/242532753_New_Tab_-_Shorter_V2_After does this one work?
21:24jawsursula: yeah that works, thanks!
21:25ursulanp :)
21:25jawsursula: oh, while i've got you for a second, i noticed that some of my top sites now just show a product icon
21:25jawsis there an API for a site to use if they want their screenshot replaced by their own icon?
21:26jawsfor example, Facebook and Reddit show the "f" and the little robot guy, respectively
21:26ursulajaws: so what you're seeing, i'm assuming, is something we made which is called tippy top https://github.com/mozilla/tippy-top-sites
21:26ursulabut we're backing that out
21:27ursulathere were some issues with legal when we did that
21:27jawsokay
21:27ursulathe idea eventually though will be to parse the page and pick out any rich icons that are provided by the site and display those instead of screenshtos
21:28ursulaso in the future you might see a combination of high res icons and screenshots for top sites that don't provide nice icons
21:29jawsyeah those match the icons in the tippy-top repo
21:30jawscool
21:30jawshopefully we can publish some type of API or criteria for this
21:30jawsthen i can update my website to use the API
21:30ursulathat would be awesome
21:30jawsit should probably go through the web standards track though
21:30jawsi'm sure chrome would appreciate it
21:32ursulajaws: yeah definitely. like right now in the test pilot addon we try and look for apple touch icons and stuff and then determine the size and sort of "guess" if it's a nice icon
21:37felipeRyanVM: halp
21:38RyanVMhi
21:41felipeRyanVM: hey, I got this try push
21:41felipehttps://treeherder.mozilla.org/#/jobs?repo=try&revision=73f82e0891f383d86e157a6fe5de6d34d8fea09e&selectedJob=122076144
21:41felipethe Windows m-clipboard tests are orange, but I don't think it's related to my patches
21:41felipedo you know if that's expected?
21:41felipelooking at the logs, it looks like those tests ran successfully
21:42RyanVMyup, expected
21:42felipeah, cool :)
21:43felipeRyanVM: thanks!
21:47RyanVMfelipe: (bug 1387831)
21:47firebothttps://bugzil.la/1387831 NEW, bugspam.Callek@gmail.com mochitest clipboard isn't running on windows anymore
22:41Callek_cloud9damnit did I break things :(
22:42Callek_cloud9ooo wiat I didn't know I landed clipboard tests....
22:43Callek_cloud9felipe: ooo duh, tier 2 on w7
22:44Callek_cloud9(which of course is ignored due to it running in buildbot)
22:44Callek_cloud9RyanVM: thanks for the poke on it ;-)
22:44RyanVMI uh, do what I can...
22:46Callek_cloud9:-)
10 Aug 2017
No messages
   
Last message: 12 days and 14 hours ago