mozilla :: #screenshots

18 May 2017
00:00cloudops-ansiblescreenshots-dev build #21: building mozilla/screenshots:latest
00:02cloudops-ansiblescreenshots-dev build #21: building failed /cc relud
00:21cloudops-ansiblescreenshots build #39: app in stage failed /cc relud
00:44GitHub[screenshots] 6a68 created lazybutton (+1 new commit):
00:44GitHubscreenshots/lazybutton 1b89e05 Jared Hirsch: Move the context menu item to bootstrap...
00:44GitHub[screenshots] 6a68 merged lazybutton into bootstrap-button-wip:
00:45GitHub[screenshots] 6a68 deleted bootstrap-button-wip at 1b89e05:
00:45GitHub[screenshots] 6a68 deleted bootstrap-button at d27dba5:
00:50_6a68localizing the toolbar button and context menu item is going to be interesting, since we're no longer just able to use webextension APIs to handle the work for us...
00:54_6a68pushed the dummy context menu item to Try, just in case
00:54_6a68that's on top of the same central commit as the Try runs we discussed earlier today
15:56cloudops-ansiblescreenshots build #40: cron in stage failed /cc relud
15:59cloudops-ansiblescreenshots-dev build #22: building mozilla/screenshots:latest
16:08cloudops-ansiblescreenshots build #41: mozilla/pageshot:7.0.0 deployed to Stage
16:10cloudops-ansiblescreenshots-dev build #22: mozilla/screenshots:latest deployed to Dev
16:13cloudops-ansiblescreenshots build #42: building mozilla/screenshots:7.0.0
16:16cloudops-ansiblescreenshots build #42: building failed /cc relud
16:35cloudops-ansiblescreenshots build #43: building mozilla/pageshot:7.0.0
17:12cloudops-ansiblescreenshots build #43: app in stage failed /cc relud
17:12GitHub[screenshots] ianb opened issue #2851: Use long-lived authentication cookies
17:14GitHub[screenshots] ianb commented on issue #2847: It occurs to me that this might integrate badly with wantsauth and the sitehelper sitehelper could be loaded when the background page is not, and so if it tries to communicate that might fail.
17:32cloudops-ansiblescreenshots build #44: mozilla/pageshot:7.0.0 deployed to Stage
18:02_6a68Sadly, the context menu run may not have regressed because some of the platforms never ran overnight
18:03_6a68if mac and windows 8 tests never ran, then there's no delta to compare
18:03_6a68still doesn't explain all the improvements, but it's an interesting data point
18:03_6a68working with jmaher in #perf to figure this out
18:11_6a68ok, TLDR: the osx queue is 14-16 hours behind, not clear why contextmenu improved perf
18:12_6a68I'm going to kick off new linux-only talos jobs with central, screenshots, screenshots with lazybutton, screenshots with lazybutton + contextmenu, and update in here when results are in
18:12_6a68also, apparently nonmain fileio is not a reliable test on Try, so that 1000+% regression is meaningless
18:37clouserw_6a68: I thought he said the base was a regression, so that explained the contextmenu "improved" perf ?
18:38_6a68I don't really understand that, because the contextmenu commit is basically the same code
18:38_6a68I figured, just rerun everything and see how it looks
18:38_6a68like, if the base was a regression, why wouldn't adding code to the base also be a regression?
18:39clouserwI thought it was just the comparison
18:39_6a68all the comparisons are from the same base commit, though
18:39_6a68or maybe I'm just missing some basic thing here, which is possible ^_^
18:39clouserwheh, possible I don't understand either :)
18:40clouserwwhat's the ETA on your new runs?
18:40_6a68kicking them off now, just running linux, hopefully have results in a couple hours
18:40_6a68after lunch, if we get lucky
18:41clouserwk, thanks. let's sync up this afternoon to write that email
18:56GitHub[screenshots] ianb created dmca-notice-language (+1 new commit):
18:56GitHubscreenshots/dmca-notice-language be2f725 Ian Bicking: Fix #2836, update DMCA owner notice
18:56GitHub[screenshots] ianb commented on issue #2836: Also updated in the [string doc](
18:56GitHub[screenshots] ianb opened pull request #2852: Fix #2836, update DMCA owner notice (master...dmca-notice-language)
19:02GitHub[screenshots] ianb created fully-qualified-ogimage (+1 new commit):
19:02GitHubscreenshots/fully-qualified-ogimage 7ebe055 Ian Bicking: Fix #2810, full qualify the homepage og:image/etc images
19:02GitHub[screenshots] ianb commented on issue #2810: Note static/homepage/install-test-local.html isn't a React page and so we can't use staticLink there -but the page is just for debugging.
19:02GitHub[screenshots] ianb opened pull request #2853: Fix #2810, full qualify the homepage og:image/etc images (master...fully-qualified-ogimage)
19:07_6a68ian was right, I think a syntax error was preventing the webextension code from running, that's why the contextmenu commit seemed more performant
19:10GitHub[screenshots] ianb created takedown-docs (+1 new commit):
19:10GitHubscreenshots/takedown-docs cb01cc6 Ian Bicking: Fix #2739, document DMCA takedown process
19:11GitHub[screenshots] ianb opened pull request #2854: Fix #2739, document DMCA takedown process (master...takedown-docs)
19:12ianbickingrelud: Im looking at and the HPKP header; is that something that should be setup by you? I dont feel like I know enough to know the proper value
19:17clouserw has the format, but we'll need to put the right certs in
19:17clouserwdoing it all on the server could make sense though
19:18clouserw(that is, have relud do it in the config)
19:18ianbickingyeah, I could do it easily enough (use some config for those public keys), but it could probably also go in the nginx config
19:18reludianbicking: that is something we can set in nginx, yes
19:19ianbickingrelud: OK, Ill assign that ticket to you
19:40_6a68hmm, the lazybutton branch isn't finding the icon and that might impact startup performance. gonna be a little while before I can start that Try job.
19:40_6a68in the meantime, we can look at the performance of the master branch against current mozilla-central:
19:41_6a68current central: -
19:41_6a68current central + master branch: -
20:41_6a68I gotta break for lunch, still debugging the icon loading issue
20:58GitHub[screenshots] ianb created change-image-domain (+2 new commits):
20:58GitHubscreenshots/change-image-domain d9f9fe6 Ian Bicking: Show error message in shot.js assert() exception
20:58GitHubscreenshots/change-image-domain 53e3591 Ian Bicking: Fix #2300, host images on config.contentOrigin
20:58GitHub[screenshots] ianb opened pull request #2855: Change image domain (master...change-image-domain)
21:01GitHub[screenshots] ianb opened issue #2856: Remove fromMakeError from Sentry reports
21:12GitHub[screenshots] ianb created sentry-metrics-docs (+1 new commit):
21:12GitHubscreenshots/sentry-metrics-docs 39a730d Ian Bicking: Fix #2837, add some more details about Sentry to the metrics documentation
21:13GitHub[screenshots] ianb opened pull request #2857: Fix #2837, add some more details about Sentry to the metrics documentation (master...sentry-metrics-docs)
22:21GitHub[screenshots] ianb created change-console-to-mozlog (+1 new commit):
22:21GitHubscreenshots/change-console-to-mozlog 2c778b1 Ian Bicking: Fix #2177, change console.* server messages to use mozlog
22:21GitHub[screenshots] ianb opened pull request #2858: Fix #2177, change console.* server messages to use mozlog (master...change-console-to-mozlog)
22:23GitHub[screenshots] ianb opened issue #2859: Review errorResponse
22:39GitHub[screenshots] 6a68 created bootstrap-button (+1 new commit):
22:39GitHubscreenshots/bootstrap-button d27dba5 Ian Bicking: WIP moving button into bootstrap.js...
22:41_6a68ianbicking: welp, I guess hacking on the file might well be necessary to get the icon to work :-(
22:42_6a68the other big question in my mind is how we're going to localize the button and context menu text
22:42ianbicking_6a68: that seems plausible, yes. rhelmer or Standard8 would know more exactly what to do I felt like I was missing several details to just understand what I was trying to do
22:43rhelmerI do (unfortunately) have some experience hacking files :)
22:43ianbicking_6a68: we could make our own half-assed localization system for that one string, to extract it out of the webextension
22:43_6a68rhelmer: hey! we need to manage the screenshots button from the bootstrap.js, not from the webextension. I've tried registering the icon using the chrome.manifest, but it's not showing up
22:44ianbickingI am happy to make a build step that creates an object {lang1: trans1, } out of the properties, then we just need to figure out the locale to render
22:44rhelmer_6a68: can I see a diff or something?
22:44_6a68I can push the branch, one sec
22:44rhelmerianbicking: you really should use your whole ass
22:45rhelmera whole-assed localization system
22:45ianbickingrhelmer: I assume its a reference to sitting on a dirty toilet seat, trying but not really succeeding in keeping your butt clean, while clearly endangering everyone else who later uses that seat
22:45_6a68rhelmer: ok, so here's the latest commit on the WIP branch
22:46rhelmer_6a68: tx
22:46_6a68rhelmer: the extra annoying thing here is that, because screenshots landed in nightly, you can't just build and install the addon, you have to rebuild firefox to test
22:46ianbickingbut for now I am off to dinner, see yall later
22:46_6a68ianbicking: cool, later
22:46rhelmer_6a68: that's ok I have to keep an up-to-date m-c tree around anyway
22:46rhelmer_6a68: actually hm why can't you just install the add-on>
22:47rhelmertemp and normal install overrides the built-in location
22:47_6a68rhelmer: hmm, I guess it should work, right?
22:47rhelmeryeah should
22:47rhelmerthat's the way upgrade works, the upgrade location overrides the built-in
22:47rhelmerso if you have bugs where you are not unregistering resources etc. you're gonna wanna fix that anyway :)
22:50_6a68yeah, this started as just exploring if the bootstrap-managed button could work, so I've been taking shortcuts
22:56_6a68rhelmer: hmm, when I install the addon, the icon is shown
22:56_6a68maybe I'm importing it into firefox wrong?
23:01rhelmer_6a68: hm, yeah it might not be getting registered just right
23:02rhelmer_6a68: hm. so are you trying to drop a chrome.manifest in ./browser/extensions/screenshots/ instead of using
23:03_6a68but only because I know how to use a chrome.manifest, and have gotten confused by in the past ^_^
23:04_6a68ian had a work-in-progress commit where he borrowed some of the pocket code, including the stuff
23:05_6a68oh wow! I found actual docs for hacking on!
23:06rhelmerhehe yeah
23:06rhelmerthey aren't bad
23:06rhelmerI have been working off-and-on on moving built-ins to the omni jar
23:06rhelmerso I've had to dig pretty deep into it..
23:06rhelmer_6a68: what you probably want is a, that'll generate the chrome.manifest for you
23:06* _6a68 nods
23:07rhelmerthere might be a way to use chrome.manifest, since you need to maintain one in your repo...
23:08_6a68oh, is that because doesn't work for extensions? because I only created this file to try to link to the icon
23:08rhelmer_6a68: the easiest way to figure out what's going on is to poke around in the objdir
23:08rhelmeryou can see what files are being put where
23:08_6a68if is the quickest way there, I'm fine to use that will work for extensions, the build system will use it to generate the chrome.manifest
23:08rhelmer_6a68: look at the pocket/ it is a pretty complex one
23:08rhelmerfirst line needs to be something like:
23:08rhelmer[features/] chrome.jar:
23:09_6a68awesome! was just going to say, I'll look at the pocket code ^_^
23:09rhelmerthe build system will turn that into a xpi, confusingly
23:09_6a68now, screenshots has been living under browser/extensions, not browser/features
23:09_6a68does that matter? I'll just put the correct path in the
23:09rhelmer_6a68: yeah, the output dir is features
23:09rhelmerit's in ./browser/extensions in the tree, but in the app dir it's ./features/
23:09* _6a68 realizes this is going to be a bumpy ride
23:10rhelmerso in an installed app (or a `./mach package`d build) they go in like /Applications/
23:10rhelmerbunch of XPIs
23:10rhelmeralthough as I mentioned I am working on moving them to omni ja so I'll probably hack up your at some point
23:11rhelmerit'll improve startup perf and also make it less convenient for people to drop junk into the app dir
23:12_6a68rhelmer: ok, so for testing, can I install as an addon, or does it need to be in the tree?
23:12rhelmer_6a68: needs to be in the tree, the build system is the only thing that understands it
23:12rhelmer_6a68: you can do e.g. `./mach build browser/extensions` and it'll be fast though
23:13_6a68awesome, I've got an artifact build going locally, so it's pretty fast
23:13_6a68I have a related question, about l10n
23:13rhelmerI've had some issues w/ `./mach package` and artifact builds but I don't think you'll need to deal with that
23:13rhelmerI still don't feel like I know how our l10n stuff works :/ but go ahead
23:13_6a68ah, I've just been doing mach build locally
23:14_6a68the strings are inside the webextension right now, but we need the toolbar button label and the context menu label from the bootstrap
23:14rhelmeryeah that's find build will do what you want for testing
23:14rhelmerthat'll generate a chrome.manifest
23:14rhelmerhrm, I'd ask in #teamaddons about the localization thing...
23:14_6a68so the question is, can I get the strings from the webextension, is it chrome addressable?
23:14rhelmerheh everything is chrome accessible
23:14rhelmeryou can shell out if you want :P
23:14rhelmeridk how convenient it'd be - John-Galt or aswan might have an idea ^
23:15rhelmer_6a68: chrome code can literally do anything
23:15_6a68I could either try to pull strings from the webextension, or change the addon build steps to put just the bootstrap-managed strings someplace different
23:15rhelmerwe have APIs to load binary code into the main process (jsctypes), execute commands, manipulate files, etc.
23:15_6a68oh yeah, that makes sense
23:15John-GaltHuh what?
23:16rhelmerJohn-Galt: screenshots is splitting part of their add-on into the bootstrap.js code, to avoid perf problems
23:16John-GaltOh, strings. Yeah, you could do that with some effort.
23:16* ianbicking thinks the build step wouldnt be that hard
23:16rhelmerwondering if they can get at the strings from the bootstrap.js
23:16John-GaltHowever, see
23:16firebotBug 1364974 NEW, Allow off-thread decoding multiple scripts for a single global in one operation
23:17rhelmerJohn-Galt: sounds like good news! what release will we enable oop extensions though?
23:17John-GaltHopefully still 55, but the regression is small enough even without OOP that it should be doable either way.
23:18_6a68oh, heh, I was just formulating the question over in #teamaddons
23:18_6a68seems like it might be easier just to build a few strings into a separate location
23:19rhelmerJohn-Galt: what about that 15% tabpaint regression?
23:20rhelmerI might be misreading what you're saying there
23:22John-Galtrhelmer: That'll be fixed soon enough either way, but it's not as bad as it looks.
23:37_6a68John-Galt: hey, since you're in here, I noticed that current screenshots master (the all-webextension code) isn't regressing very much vs current m-c:
23:38_6a68do those numbers seem accurate to you? aside from tp5o, they are all within at most 6-7% of baseline
23:39John-Galt_6a68: Yeah, bug 1361900 landing should have helped with the startup numbers
23:39_6a68I'm curious how much is left to optimize for 55 (since moving the button into bootstrap is a wee bit complicated)
23:39firebot FIXED, Use script precompiler in content processes
23:39John-Galt_6a68: See
23:39firebotBug 1364974 NEW, Allow off-thread decoding multiple scripts for a single global in one operation
23:40_6a68John-Galt: hey, that's great! so basically, just need to land OOP extensions to get Talos within acceptable limits?
23:41John-GaltNo, just need to land bug 1361900 to get most of the numbers within acceptable limits.
23:41John-GaltWith a mostly empty extension, anyway. Haven't tested with screenshots.
23:46rhelmerJohn-Galt: does the empty extension have a background page, or just manifest?
23:46John-Galtrhelmer: An empty background page
23:53clouserw_6a68: draft @
23:54_6a68clouserw: that looks great. I can fill in some Try links
23:55clouserwcool, thanks
19 May 2017
No messages
Last message: 92 days and 17 hours ago