mozilla :: #fx-team

26 May 2017
08:15mikedeboerpaolo: that looks like an unintentional side-effect of my patch
08:16mikedeboerpaolo: I think my main goal there was to make sure that the panel content (buttons, etc) hug the edges of the panel, so that hover-effects are full-width
09:00Gijsmikedeboer: hey! You around today? :)
09:59mikedeboerGijs: absolutely!
09:59mikedeboer(he says, an hour later :P )
09:59mikedeboergrinding through bugmail and the like
09:59mikedeboerbusy day!
10:00Gijsmikedeboer: excellent. I've been looking at starting off a library panel, and running into some issues
10:00mikedeboerissues are not nice
10:00Gijsmikedeboer: basically, it needs a photonpanelmultiview because it has subviews that then have subviews
10:01Gijsmikedeboer: I'm relying on PanelUI.showSubView, as we do with other type=view widgets, and I've taught it to use a photonpanelmultiview when the photon pref is flipped
10:01Gijsmikedeboer: however... after I open that (library) panel, the main hamburger panel starts also showing the library view :|
10:01Gijsmikedeboer: I haven't worked out how this is possible...
10:02Gijsaren't there two binding instances, and therefore two instances of PanelMultiView objects, and therefore 2 instances of the panelViews sliding linked list thingy?
10:02* Gijs is very confused why things are broken :(
10:02mikedeboerGijs: I think that's not what you want right now, because there are so many panels/ widgets that use this mechanism right now
10:03mikedeboerso I think it'll break/ convert too many things to photonpanelmultiview right now
10:03Gijsmikedeboer: it's conditional on the photon pref, it's fine - I don't intend to ship it, but we need to move it over eventually
10:03mikedeboeryeah true
10:03Gijsand this is only for the case where we open things in a temp panel
10:03Gijsso it's basically only buttons that open subviews in the toolbar
10:03mikedeboerok so it looks like the two instances mux things up?
10:04Gijsyeah, somehow
10:04Gijswould it help if I just pastebin the patch or something?
10:08mikedeboerGijs: looking into it right now, btw
10:10mikedeboeryields the same problem for the Pocket b utton
10:13Gijsmikedeboer: that's odd... I didn't think pocket even used panel views?
10:13GijsI guess I'm wrong! :)
10:32mikedeboerGijs: how are temp panels removed/ cleaned up?
10:33Gijsmikedeboer: I think onpopuphidden we remove the panel
10:33mikedeboerok, I see the impl. noe
10:34Gijswe also append the kiddo back to the main appmenu's multiview
10:34Gijsso I somewhat suspect that's the culprit
10:34Gijsbut I don't understand *why* that would be the case
10:34mikedeboerwhy on earth is that doing 'this.multiView.appendChild(viewNode)' ?
10:35mikedeboerGijs: see line 480 is the culprit
10:36mikedeboerin PanelUI.js
10:39mikedeboer(with your code applied, that is)
10:39Gijsmikedeboer: because otherwise that node will be removed...
10:39Gijslike, it's kid of the panel, and nothing will have a reference to it afterwards, and we're removing the panel
10:39Gijsso it needs to *somewhere*
10:39Gijsand appending it to the multiview would seem like it's the right thing to do in the old panelmultiview
10:40Gijsif that changed for the new binding we should come up with a different thing to do :)
10:43mikedeboeryeah, since now it's a stack... one sec, I'm testing a fix
10:46mikedeboerGijs: your thing withg my fix:
10:47Gijsmikedeboer: what is the original parent in the photon case?
10:47mikedeboerI don't know if there's a case when the view never had a parentNode to begin with
10:48Gijsmikedeboer: I'm asking because I don't think we can guarantee that the originalSibling is in the same place, either
10:48Gijsmikedeboer: come to think of it... if we open 2 or 3 subviews in this panel, we'll need to make sure all of them are put somewhere reasonable again...
10:50mikedeboerGijs: hmm... atm pocket is doing a bunch of custom stuff as well... hardcoding on '"PanelUI-multiView"' being there
10:51Gijsmikedeboer: we'll have to fix that separately I guess
10:52mikedeboerGijs: so basically we should formalize this 'adoption' process in PanelMultiView.jsm, not like this
10:52Gijsmikedeboer: that makes sense. Where's a good place in the new photon binding? Then I can probably deal with it :)
10:52mikedeboerGijs: something like: ok, when removed, stash it away in some hidden container
10:53Gijswhen is "when removed", and how would we notice? :)
10:53mikedeboerpanel.remove() calls the destructor, which may be soon enough to keep the views somewhere safe
10:54Gijsthat makes sense, actually
10:54Gijsthe only question is how to figure out the "somewhere safe" bit
10:54pasthas anyone else seen session restore not playing well with redirects on nightly?
10:55mikedeboerGijs: document.documentElement.appendChild(document.createElement("box")).hidden = true; ?
10:55Standard8past: in what way?
10:55mikedeboerwhen temporary=true on the panel... or something
10:56mikedeboeror do it always...
10:56pastas in the tab displays the original url, not the target of the redirect
10:56mikedeboerpast: I haven't, but if you can reproduce it reliably I'd be really happy if you filed it!
10:57pastI've seen it now in two systems on my main profiles, but I'll see if I can reproduce in a clean profile
10:58Standard8past: there was a regression filed on something like that a few days ago, lemme see if I can find it
11:00Standard8past: bug 1366203 may be relevant, but not the one I was thinking of
11:00firebot NEW, Changing URL locations while a page is loading temporarily reverts the URL
11:07pastmikedeboer: it was already filed, bug 1367827
11:07firebot NEW, Redirected link restored to the source and not target of the link
11:08mikedeboeroh, haven't gotten that for in my bugmail stack yet
11:08pastpoor man, I feel for you
11:08* past pats mikedeboer in the back
11:09mikedeboerhaha lol - at least I had a nice day off yesterday ;)
11:16Gijsit sounds different from 1366203...
11:16Gijsmikedeboer: hm, I guess...
11:18Standard8fwiw bug 1319111 has just been backed out, so it might be worth retesting things in todays nightly
11:18firebot ASSIGNED, Expose result principal URL ("final channel URL") on LoadInfo, convert current consumers of LOAD_REP
11:25glandiumdo we have a helper function in js equivalent to python's string.rstrip? (remove occurrences of the given character at the end of the string)
11:26glandiumlike trimRight, but for arbitrary characters
12:28mikedeboerglandium: " somestring Xgarb ".replace(/Xgarb[\s]*$/, "")
13:24pastlooks like the session restore bug is fixed in the latest nightly
13:56mikedeboerGijs: do you have an up-to-date local build? If so, can you yank 'AppMenuNotifications.showNotification("update-restart", {callback() {}}, null, {dismissed: true})
13:56mikedeboer' in there for me?
13:57Gijsmikedeboer: I don't, though I'm in the process of building one...
13:57Gijsmikedeboer: why?
13:57mikedeboerGijs: expected result: see a truncated standard label and a fully in-view multiline one
13:57Gijsthat sounds odd...
13:57Gijshow did we break that? :(
13:57mikedeboerI swear!
13:58Gijsmikedeboer: and this doesn't happen on today's nightly?
13:58Gijsah yeah, it does, I can reproduce it there
13:58Gijshappens on Nightly for the 25th, even
13:59Gijsmikedeboer: do you want to file, given you noticed it?
13:59mikedeboerGijs: but that's also for non-photon?
13:59Gijsoh, does it?
13:59mikedeboerno, I'm asking
14:00Gijsno, non-photon looks OK
14:00mikedeboerGijs: so how are multiline labels supposed to work anyways? It seems kinda magic (haven't dug into text.xml - hoping this'd be faster)
14:01Gijsmikedeboer: not sure what you mean by 'how'...
14:01Gijsbasically, we xbl inherit them into the textContent of a XUL <label> element
14:01Gijstext in those wraps
14:01mikedeboerGijs: well, what decides when to show the multiline one or the standard one?
14:01Gijssetting the wrap=true attribute on the node
14:01Gijswould hide the normal one and show the multiline one
14:01Gijsotherwise, the other thing happens
14:03Gijsif we&#39;re force-setting display: -moz-box on the single-line label somewhere/somehow, that&#39;d explain this
14:03mikedeboerno magic after all :( I&#39;m still hoping to find it someday in some codebase
14:06GijsI wonder how that regressed...
14:06pastfelipe: hey, do you know how soon after we ship a new release we can push system add-ons to users?
14:09Gijsmikedeboer: so... based on the CSS, my educated guess is that previously, the main view would have the mainview=true attribute
14:10Gijsmikedeboer: and that must have regressed recently
14:10mikedeboerI think with paolo&#39;s patch
14:11Gijsyeah, I expect so... not sure how/why though
14:11Gijsthat might have broken other things
14:11Gijs(the attribute not being there, I mean)
14:12Gijsmikedeboer: meanwhile, related but different question... how do I identify views in the photonpanelmultiview binding that we need to &quot;rescue&quot; into some other box from the destructor?
14:12Gijsmikedeboer: this.node.children isn&#39;t it, AFAICT
14:12mikedeboerGijs: if (this.panelViews) this._viewStack.children
14:13felipepast: I don&#39;t think there&#39;s a minimum interval. As soon as needed I&#39;d say
14:14pastfelipe: so if there is a system add-on that is not in the tree and we ship it in both the old and new release, how long do you think users would be without it?
14:15pastbecause AIUI the update will remove the add-on
14:16felipepast: until their next update check. IIRC it&#39;s on a 24hr timer (there&#39;s a bug to make it happen sooner). So I&#39;d say 1 - 3 days without it
14:17pastfelipe: that is very useful, thanks
14:19Gijsmikedeboer: I don&#39;t see the destructor running...
14:20Gijswhich isn&#39;t entirely surprising, xbl is crap
14:21mikedeboershow on earth are we not leaking the world then? Do we rely on automatic GC for these things?
14:22GijsI mean, the node&#39;s event listeners go away, and CC picks up the rest, I guess?
14:23Gijsbut I tried both a breakpoint and Cu.reportError and it&#39;s not running.
14:26Gijsoh, hm
14:26Gijsthough now I added an explicit call from PanelUI
14:26Gijsand a debugger; statement in the destructor() code, because it&#39;s not doing what I expect
14:26Gijsand stepping through that doesn&#39;t work
14:26Gijsbecause of course it doesn&#39;t
14:26* Gijs flips a coin to decide what&#39;s breaking this: XBL or the debugger
14:31Gijsthis is mad... the explicit call is firing, but Cu.reportError() in there doesn&#39;t work
14:33mikedeboerand dump? or you can&#39;t use dump with your build?
14:34GijsI tried that too, doesn&#39;t work either, nor does this.window.console.error()
14:37Gijsso um, ./mach build faster is updating files on my windows machine
14:37Gijsbecause the files display correctly in the debugger
14:37Gijsbut the JS engine is running the old ones
14:37Gijswhich is why none of my logging worked
14:37Gijsand why breakpoints look broken
14:37* Gijs is going to need a drink soon.
14:39pastis that with -purgecaches?
14:39Gijswhy would I need that?
14:39Gijs./mach run should just work
14:39Gijsit normally does
14:39pastdunno, I never use &#39;mach run&#39; without it
14:40Gijsfeels pretty broken to me anyway... if you update your copy of Firefox, do we just keep reusing the caches unless you start with that flag?
14:40* Gijs sure hopes not
14:43Gijsit worked after I ran ./mach build browser/components
14:43mikedeboerGijs, woa.
14:43Gijsso I think something&#39;s just breaking ./mach build faster
15:01GijsI hate XUL. :(
15:01Gijspop quiz time: what does xulnode.children return ?
15:01Gijswell, really s/XUL/XBL/
15:01Gijsbut, you know, small potatoes (also: that&#39;s a hint, folks!)
15:04Gijsso apparently, if you ask a XBL anon node that contains the XBL <children/> annotation for node.children, it returns an HTMLCollection including a node representing the <children/> node. :|
15:13Gijsmikedeboer: so the destructor fires when the window is destroyed, it looks like
15:15nalexanderGijs: past: that does sound a lot like XUL caching. Any more details?
15:15Gijsnalexander: this is a jsm, so I don&#39;t think that&#39;s in the XUL fastload file
15:15mikedeboerGijs: ok, so you need to start listening for DOMNodeRemoved in the JS instance, or is that too late?
15:16Gijsmikedeboer: tbh, I was just going to explicitly call the destructor just before removing the panel in the panelRemover code
15:16Gijsmikedeboer: I don&#39;t think we need a generic solution - I don&#39;t think we generally create and destroy these multiviews routinely, except for this purpose (and with this code)
15:17Gijsalso mutation events are performance nightmares, I hear
15:17Gijsnalexander: I asked in #developers, but I thought I heard rumours somewhere that we were removing JS startup caching
15:17Gijswhich I think this falls under
15:17Gijsand I wonder if it&#39;s related to that somehow
15:19nalexanderGijs: I believe it is related. I dug into this and found a XUL-named pref controlled (sort of) the JS startup cache.
15:20nalexanderGijs: I added code to force purgecaches, and I see similar things in devtools now:
15:20mikedeboerGijs: also good :)
15:21* andym seems to have lost his awesome bar lookups in nightly
15:21nalexanderGijs: and (Android equivalent)
15:21nalexanderandym: did you really need them?
15:22andymnalexander: erm, just a little
15:24pastandym: do you get no results where previously you would have got some?
15:25pastGijs: were you thinking of bug 1351071?
15:25firebot FIXED, Disable startup cache generation
15:25Gijspast: yes!
15:25andympast: yes
15:25pastGijs: I think that bug only stops shipping it, not generating it on startup
15:26Gijsnalexander: so sounds like ./mach build faster should drop a &#39;.purgecaches&#39; file, and doesn&#39;t?
15:26Gijs(anymore, maybe)
15:26pastandym: I wonder if your results are now mostly search suggestions pushing history results out
15:41Gijsmikedeboer: remind me how we hide/show the header in the panelviews?
15:41* Gijs is trying to find it and failing
15:42mikedeboerGijs: in CSS, depending on whether a &#39;title&#39; attribute is set on the panelview
15:42Gijsmikedeboer: where is that CSS? :)
15:42Gijsah, browser.css
15:42* Gijs had looked in panelUI.css (in contnet)
15:43nalexanderGijs: re dropping .purgecaches, I don&#39;t think it&#39;s that simple: not every build should purge the caches. (Right?)
15:44Gijsnalexander: why not?
15:44nalexanderGijs: we definitely don&#39;t want to invest in having the build system _know_ when to purge caches; that&#39;s way down the list.
15:44mikedeboerGijs: not in content/PanelUI.css?
15:44Gijsmikedeboer: mm
15:44nalexanderGijs: wouldn&#39;t XUL/XHTML/CSS changes unnecessarily purge the caches?
15:45Gijsnalexander: yes. Is purging the caches *that* expensive?
15:45nalexanderGijs: we went with &quot;-purgecaches if !MOZILLA_OFFICIAL&quot; in Android for simplicity. Probably the right thing to do (at Firefox startup!) for local developers too.
15:45nalexanderGijs: that I can&#39;t say. I know that I was worried about perf when I did it, and had no idea how to answer that question. Sounds like the ticket past links would be the place to start digging.
15:46nalexanderGijs: my take is that .purgecaches is outdated; we should bake the logic into the startup cache code.
15:46Gijsmikedeboer: so I want to switch it to panelview[mainview] > .panel-header
15:46nalexanderGijs: but I don&#39;t have the full story here.
15:46Gijsmikedeboer: which I can&#39;t because of the exact bug you ran into earlier :P
15:46Gijsmikedeboer: guess I get to fix that too...
15:46mikedeboerGijs: I fixed it as well?
15:47Gijsmikedeboer: oh, did you file a bug / land a patch somewhere?
15:47* Gijs will r+ post-haste
15:47mikedeboerGijs: no, I&#39;ve coalesced it into bug 1364738
15:47firebot ASSIGNED, [Photon] Update notification causes scroll bar in hamburger panel menu if the update label&#39;s text wr
15:50Gijsah, hm
15:50* Gijs ponders
15:50Gijsmikedeboer: OK, then I&#39;ll just pretend that my patch will land after yours :)
15:50Gijsmikedeboer: is it an easy fix I can apply locally?
15:50Gijsmikedeboer: just for testing?
15:52mikedeboerGijs: replace the body of `setMainView(aNewMainView)` with
15:52andympast: that sounds right, that&#39;s me looking for a proxy bug i was reading the other day
16:00Gijsmikedeboer: fab, thanks
16:00mkaplyOK, so the cookie APIs warn that you should use originAttributes and point to MDN where there is no documentation. I&#39;d love to update the docs, but I can&#39;t figure out what originAttributes even are on a cookie info request. I updated the bug to say &quot;we really need to document this&quot; but there is no response.
16:00mkaplyso what do &quot;originAttributes&quot; look like? (API is cookieExists on a domain)
16:00Gijswhen you pass them from JS, they can be a vanilla JS object
16:01Gijsmostly right now they capture userContextId (containers - see test pilot) and maybe private browsing state, I forget how far integrating that has come at this point
16:01mkaplySo just {}?
16:01Gijswell, then you get the default container and no private browsing...
16:01Gijsit depends why you&#39;re calling which APIs
16:01Gijswhat are you actually trying to do?
16:02mkaplyI want to ask globally if a cookie exists for a given domain
16:03Gijsfor what purpose?
16:03Gijsie who&#39;s asking?
16:03mkaplySo if I wanted to send originAttributes from a frame script via the JSON, what would that look like?
16:03Gijsif you&#39;re asking for a particular page, reuse that page&#39;s origin attributes
16:03mkaplyTrying to determine if Bing has set a particular cookie
16:04mkaplyThis is the problem. The concept of originAttributes is completely undocumented on MDN. Is it document.originAttributes?
16:04GijsI still don&#39;t know how to help - why are you trying to determine that?
16:04Gijsit&#39;s a property on principals, among other things
16:05Gijsso document.nodePrincipal.originAttributes
16:05Gijsof course, if you do that on a system privileged thing it won&#39;t help you...
16:05Gijs(in that you&#39;ll just get the default set - the system principal is a singleton)
16:05pastandym: you might want to try disabling search suggestions in the location bar from about:preferences
16:06andymhmm, now i&#39;m getting no results at all
16:06Gijsmkaply: so broadly speaking, content.document.nodePrincipal.originAttributes may be what you want. browser.xul&#39;s document.nodePrincipal.originAttributes is not what you want
16:07mkaplyGijs: Cool, that&#39;s helpful. Although it suddenly occurs to me that I could probably just look at document.cookie in the frame script.
16:07Gijsif you have a bing document, yes
16:08Gijsmkaply: I still have no idea what you&#39;re needing to determine this for / what context this code is running in... :)
16:10pastandym: so it looks like there is indeed no match in your history. mak may have some ideas as to why that might be. Any errors in the browser console btw?
16:13andympast: no
16:15Gijsthis smells like a busted places db
16:25Gijsthere&#39;s some diagnostics that mak would know how to run
16:26Gijsor running PRAGMA integrity_check with the sqlite manager add-on
16:26* Gijs needs to dash
16:27Gijsmikedeboer: will see if I can review your patch tomorrow, otherwise monday - sorry for the delay! :)
16:27* Gijs goes for his friday evening
16:27mikedeboernp :)
17:02bsmedbergI&#39;m feeling rusty: 4 mistakes in a 21-line patch.
17:40RyanVMjaws: do you want to fix before it gets backed out?
17:40jawsyes, looking
17:40jawsRyanVM: ^
17:42bsmedbergmconley, if it&#39;s possible to do that plugin infobar review today that would be really helpful so I can collect stats over the weekend
17:42mconleyon it
17:42jawsRyanVM: i will push the fix to inbound. thanks
17:46RyanVMjaws: np,thanks
26 May 2017
Last message: 59 minutes and 43 seconds ago