mozilla :: #screenshots

19 Apr 2017
00:12GitHub[screenshots] mozilla-pontoon pushed 1 new commit to master:
00:12GitHubscreenshots/master 3233e57 Bjrn I: Pontoon: Update Norwegian Nynorsk (nn-NO) localization of Firefox Screenshots...
00:18circleci-botFailed: mozilla-pontoon's build (#1101; push) in mozilla-services/screenshots (master) --
03:09GitHub[screenshots] fangshih commented on issue #2701: Just updated the icon and toolbar to the latest, Thanks! ...
03:11GitHub[screenshots] fangshih commented on issue #2701: Just updated the icon and toolbar to the latest, Thanks! ...
03:54GitHub[screenshots] dannycoates force-pushed content-disposition from 7990bff to 3289f81:
03:54GitHubscreenshots/content-disposition 3289f81 Danny Coates: use Content-Disposition for downloading images
06:25GitHub[screenshots] dannycoates created dmca (+1 new commit):
06:25GitHubscreenshots/dmca 879ee99 Danny Coates: added DMCA notice to shot page
06:32GitHub[screenshots] mozilla-pontoon pushed 1 new commit to master:
06:32GitHubscreenshots/master efdf847 Mohammad Yaseen Khan: Pontoon: Update Urdu (ur) localization of Firefox Screenshots...
06:38circleci-botFailed: mozilla-pontoon's build (#1104; push) in mozilla-services/screenshots (master) --
06:40circleci-botFailed: dannycoates's build (#1105; retry by dannycoates) in mozilla-services/screenshots (content-disposition) --
09:00GitHub[screenshots] SoftVision-CosminMuntean commented on issue #2698: > We might want to see if @SoftVision-CosminMuntean can repro on other channels before figuring out a workaround....
11:11GitHub[screenshots] mozilla-pontoon pushed 1 new commit to master:
11:11GitHubscreenshots/master 939b7bf : Pontoon: Update Telugu (te) localization of Firefox Screenshots...
11:17circleci-botFixed: mozilla-pontoon's build (#1106; push) in mozilla-services/screenshots (master) --
11:56GitHub[screenshots] PRiMENON commented on issue #2256: Thanks a lot!
12:23GitHub[screenshots] SoftVision-CosminMuntean opened issue #2707: A Screenshots error is displayed, if you drag a tab back in the first browser window
12:42GitHub[screenshots] SoftVision-CosminMuntean opened issue #2708: The Screenshots button is not grayed out when it is inactive (Windows and Linux)
13:53GitHub[screenshots] SoftVision-CosminMuntean commented on issue #2599: This issue is no longer reproducible, but if a selection with reduced width is performed near the left part of the window, the "Cancel", "Download" and "Save" buttons are not entirely visible. ...
15:00GitHub[screenshots] SoftVision-CosminMuntean commented on issue #2681: I ran another round of testing today on the latest Nightly build and I found a couple of issues....
15:33Standard8ckprice: ianbicking: any objections to me enalbing the screenshots pref on nightly?
15:33Standard8aka should I land
15:33firebotBug 1356243 NEW, Enable Screenshots for FF 55 & beyond
16:02GitHub[screenshots] Standard8 commented on issue #2348: Current list of failing tests...
16:04ckpriceStandard8: does the timing on that landing have to be close to us uplifting? if not, maybe we can let the beta dust settle
16:04Standard8ckprice: nope, it just affects when we get more users looking at it on nightly builds
16:07ckpricecool. we have a stand up in a bit, we'll dig into the severity of the new bug SV posted. let's not land it today
16:07ckpriceStandard8: ^
16:07Standard8ckprice: ok, just send me mail or comment on the bug when you want it landed
16:11GitHub[screenshots] ianb force-pushed selenium-tests from f6f13eb to 3ab04e9:
16:11GitHubscreenshots/selenium-tests 3ab04e9 Ian Bicking: Increase test timeout, plus debugging console.log
16:16GitHub[screenshots] Standard8 commented on issue #2348: Current list of failing tests...
16:17circleci-botFailed: ianb's build (#1107; push) in mozilla-services/screenshots (selenium-tests) --
16:18Standard8ianbicking: _6a68: fyi, Ive been filing bugs that affect us for enabling screenshots in tests against bug 1355569 - the ones Ive concentrated on so far are ones I think well need outside help for.
16:18firebot NEW, [meta] Fix unit tests broken when Screenshots is enabled.
16:19Standard8 1356256 Ill look at getting _6a68s patch up on tomorrow
16:19Standard8and 1357797 probably is actually a non-issue but I put it there for now as it shows up on try server
16:19Standard8The rest of the issues Ill get filed tomorrow, and hopefully fix a few
16:22_6a68Standard8: Cool, thanks!
16:22circleci-botFailed: ianb's build (#1109; retry by ianb) in mozilla-services/screenshots (selenium-tests) --
16:24GitHub[screenshots] dannycoates force-pushed content-disposition from 3289f81 to defb4b2:
16:24GitHubscreenshots/content-disposition defb4b2 Danny Coates: use Content-Disposition for downloading images
16:31GitHub[screenshots] 6a68 commented on issue #2708: Hmm. The button is actually [supposed]( to be enabled on about:blank and about:newtab, as well as the activity stream newtab page. If I manually load those URLs, the button is enabled, and clicking the button goes to, as intended.... ht
16:39GitHub[screenshots] dannycoates opened pull request #2709: added DMCA notice to shot page (master...dmca)
16:40GitHub[screenshots] SoftVision-CosminMuntean commented on issue #2708: > The button seems to be correctly disabled on other about: pages, like about:config, about:performance, about:addons....
16:46GitHub[screenshots] 6a68 commented on issue #2708: > on Windows and Linux the button is not grayed out...
16:46GitHub[screenshots] dannycoates commented on issue #2698: who even drags the scrollbar anymore besides QA folk? (my dad probably)
16:46GitHub[screenshots] 6a68 commented on issue #2708: @SoftVision-CosminMuntean Sorry, I just realized this _was_ the bug, I missed the title :-P
16:47GitHub[screenshots] 6a68 commented on issue #2708: Putting back the gamma blocker tag, sorry for the bug churn
16:48GitHub[screenshots] 6a68 opened issue #2710: Enable button on about:home
16:51GitHub[screenshots] dannycoates commented on issue #2706: exceptionel
16:51557A68XI5[screenshots] ianb closed issue #2600: The "Download" button from saved shot details page is not correctly working (stage)
16:51203A8FQY1[screenshots] ianb closed pull request #2693: use Content-Disposition for downloading images (master...content-disposition)
16:51GitHub[screenshots] ianb pushed 1 new commit to master:
16:51GitHubscreenshots/master 70b2a43 Ian Bicking: Merge pull request #2693 from mozilla-services/content-disposition...
16:51GitHub[screenshots] ianb deleted content-disposition at defb4b2:
16:52GitHub[screenshots] ianb closed issue #2699: Clicking the "Terms" and "Privacy Notice" links from Onboarding tour, open a new blank tab
16:52GitHub[screenshots] ianb deleted 2699-fix-terms-and-privacy-urls at e8c5809:
16:54GitHub[screenshots] dannycoates force-pushed dmca from 879ee99 to d66143f:
16:54GitHubscreenshots/dmca d66143f Danny Coates: added DMCA notice to shot page
17:00circleci-botFailed: dannycoates's build (#1114; push) in mozilla-services/screenshots (dmca) --
17:21circleci-botFixed: dannycoates's build (#1115; retry by dannycoates) in mozilla-services/screenshots (dmca) --
17:43GitHub[screenshots] 6a68 commented on issue #2708: It turns out that it's a known issue that WebExtension buttons aren't visually changed when disabled on Windows and Linux :-\...
17:45GitHub[screenshots] ianb commented on issue #2709: Something that we're planning on for new updates (that we haven't done before) is to make the code tolerant of different database versions. So for our purposes, 6.3.0 is the current production and runs with database version 15. We should ensure that 6.3.0 can run with database version 16, and that master can run with database version 15. *Or*, if we can't do that, then we are supposed to releas
18:03_6a68weirdly, some of them are smaller than others for me
18:11GitHub[screenshots] 6a68 commented on issue #2704: I can't find the error now, but I'm pretty sure I saw an error in today's nightly about failing to update system addons. Can you repro this on dev edition or a slightly older Nightly?
18:13GitHub[screenshots] 6a68 commented on issue #2704: Hmm, here's a bug about system addons not getting updated, but it's labeled as fixed:
18:13firebotBug 1350064 FIXED, System add-ons deployed with a build do not update an existing system add-on that was pushed.
18:13rhelmer_6a68: re ^ nightly doesn't really use system addon updates
18:14rhelmerI'd be curious to see the error but I wouldn't be too worried
18:14_6a68rhelmer: could you take a look at the bug? the test is trying to change the installed addon, I think, but it's failing
18:16rhelmerhm. ianbicking I am confused about that ^ are you installing an addon over the built-in system add-on?
18:16ianbickingrhelmer: yeah, Im installing the in-development version of the add-on over the system add-on
18:17rhelmerianbicking: is that via a temp addon in about:debugging or installing in about:addons or something else?
18:17ianbickingrhelmer: this is the code we use to install:
18:18rhelmeryeah so AddonManager.installTemporaryAddon() is the API that about:debugging uses, so it's loading it as a temporary addon
18:19rhelmerianbicking: this seems more likely to be the original add-on not freeing resources or the new one not overriding them... it could be related to the way temporary add-ons are installed, I'm not sure if they do the whole "upgrade" process
18:19rhelmerianbicking: can I get the addon this build produces to try to repro this locally?
18:21ianbickingrhelmer: the zip
18:21rhelmerianbicking: instead of installing this as a temporary add-on over the top of the system add-on you might want to use the system addon upgrade mechanism... you'd need to produce an XML file and flip a pref
18:21rhelmerthen call the AddonManager API that does the update check
18:22rhelmerdepends what you're trying to test, I guess. installing temp over the top is good for debugging and hacking on
18:22ianbickingrhelmer: it can look at a local XML file for the update?
18:22rhelmerianbicking: see the pref extensions.systemAddon.update.url
18:22rhelmerwell I point it at localhost, idk if a file:// URL would work but maybe
18:23rhelmerok thanks I'll try that addon on nightly
18:23rhelmer_6a68: bug 1350064 is about updates sticking around after an app update, so it's like the opposite of the problem you're seeing :)
18:24firebot FIXED, System add-ons deployed with a build do not update an existing system add-on that was pushed.
18:24ianbickingrhelmer: that seems a bit complicated and fragile, if theres a way to make the more imperative way work that would be nice
18:24rhelmerianbicking: like have an API you can just pass an nsIFile to, like the way install temporary works?
18:25rhelmerthat would be easy to add, like AddonManager.installSystemAddon() that returns a promise etc
18:25ianbickingrhelmer: yeah well, should installTemporaryAddon work?
18:25ianbickingrhelmer: since we want to support 54, wed ideally want an API that works with 54, so maybe a new method isnt so useful :(
18:25rhelmerianbicking: well, I am not sure what you expect it to do. should installing a temporary add-on over an existing one be an "upgrade"?
18:26rhelmerI'm not sure what you're intending to test
18:27ianbickingrhelmer: as were doing development, we want to run integration tests with the in-development add-on and multiple Firefox versions
18:27rhelmerok makes sense. well let me look and see if I can repro locally
18:27ianbickingrhelmer: on the CI server it seemed intermittent, but its been consistent for me locally
18:33ianbickingrhelmer: oh, it occurs to me that bootstrap.js:shutdown is probably not stopping the webextension properly?
18:34rhelmerianbicking: that is what I am wondering.. I think temporary load calls shutdown on the underlying add-on but I don't know if it calls uninstall (the underlying one is disabled not uninstalled)
18:34ianbickingrhelmer: Im wondering if the embedded webextension isnt being shutdown properly, since AFAICT theres nothing to shut it down
18:35_6a68Yeah, looks like adding prefs to about:preferences indeed requires hacking on XUL files. here's one such file
18:36ianbicking_6a68: is it possible from bootstrap.js?
18:36_6a68here's a patch from Tor folks that would add a checkbox to one of the preferences screens
18:36firebot NEW, checkbox in about:preferences#privacy for privacy.resistFingerprinting (Tor 20244.1)
18:36_6a68ianbicking: I'm guessing not, but I suppose all things are possible with XUL
18:36_6a68ianbicking: it seems like the XUL preferences screen is localized, and I'm not sure how we could inject strings into that
18:38_6a68I'll ask mconley, looks like he reviews a lot of patches in the preferences code
18:38rhelmerhm, looks like it's not doing anything on uninstall, shutdown should be called..
18:40GitHub[screenshots] dannycoates commented on issue #2707: The notification is already fixed by #2702 ...
18:42GitHub[screenshots] dannycoates closed issue #2707: A Screenshots error is displayed, if you drag a tab back in the first browser window
18:44rhelmerianbicking: ok, yeah I can repro. so here's one thing - if I load the addon as temporary before I flip the flag to enable the built-in one, then it uses the localhost url to save screenshots
18:45ianbickingrhelmer: OK, cool, that makes sense Ill make a patch to bootstrap.js to call webExtension.shutdown() when bootstrap.js:shutdown() is called
18:45rhelmerif the built-in has been activated, then I can repro... which makes me think it's either specific to the add-on or possibly webextension specific, but I need to look over the code
18:45rhelmeryeah that could be it
18:45ianbickingand then I can futz with the test as a temporary workaround
18:45ianbickingrhelmer: bootstrap.js *does* keep the webExtension shut down properly if the pref is flipped off, so this all makes sense I think
18:45rhelmerah, yeah webExtension.shutdown(); is not called on shutdown
18:45rhelmerI think it does
18:46rhelmerso you're installing the bootstrap part over the old one, but the webextension from before is still running
18:46rhelmerI bet this would work correctly on restart, but since it's a temp addon it goes away on restart :)
18:46_6a68note that we had to remove the 'is embedded webextensin' bit from install.rdf because of test failures
18:46rhelmerI'll check real quick that that fixes it. I guess you can call your stop() function.
18:46ianbickingyep, makes testing slightly harder too
18:46_6a68so it's possible some nice code that manages the webextension part isn't getting run
18:46rhelmer_6a68: yeah, right
18:47rhelmerwebextensions just do too much stuff to run right at startup
18:47rhelmeronce they are in their own process it should get less buggy but still might be slow in some cases, we need to do it async in any case. /digression
18:55_6a68Turns out you _can_ dynamically add to the preferences pane, and MattN showed me how it's being done by the form autofill system addon:
18:56_6a68We'll have to add some xml stuff to the non-webextension part of the addon, but should be doable without landing patches in m-c outside the addon. I'll file a github bug for it
19:00GitHub[screenshots] 6a68 opened issue #2711: Add Screenshots disable checkbox to about:preferences
19:14MattN_6a68: Note that the autofill code doesn't handle cleanup if the extension is disabled or updated while prefs are open so you'll need to handle that if it's a possibility
19:14_6a68Thanks for mentioning that. I'll add it to the bug
19:19rhelmerianbicking: _6a68: ok yeah just tried on a new m-c build - if I stop the screenshots webextension at bootstrap shutdown in the built-in, then loading a temporary add-on seems to work correctly
19:19rhelmerwhere "correctly" means "it tries to post the screenshot to localhost", that's as deep as I looked :)
19:33GitHub[screenshots] rhelmer commented on issue #2704: We chatted about this in IRC - the tl;dr is that bug 1350064 is unrelated (it's about updates sticking around after application upgrade) - the problem is that the WebExtension started by the built-in add-on is not shut down when the bootstrapped add-on is....
19:33firebot FIXED, System add-ons deployed with a build do not update an existing system add-on that was pushed.
19:54GitHub[screenshots] ianb force-pushed selenium-tests from 3ab04e9 to 67b9b52:
19:54GitHubscreenshots/selenium-tests 67b9b52 Ian Bicking: Increase test timeout, plus debugging console.log
19:56GitHub[screenshots] ianb created shutdown-in-bootstrap (+1 new commit):
19:56GitHubscreenshots/shutdown-in-bootstrap 238f0fc Ian Bicking: Shutdown the embedded webExtension when bootstrap is asked to shutdown
19:57GitHub[screenshots] ianb opened pull request #2712: Shutdown the embedded webExtension when bootstrap is asked to shutdown (master...shutdown-in-bootstrap)
19:58GitHub[screenshots] rhelmer commented on issue #2712: lgtm, this is almost exactly what I tested locally.
19:58GitHub[screenshots] ianb created firefox-54-compat (+1 new commit):
19:58GitHubscreenshots/firefox-54-compat 916af99 Ian Bicking: Add third argument to add/removeObserver, which is required in Firefox 54
19:59GitHub[screenshots] ianb opened pull request #2713: Add third argument to add/removeObserver, which is required in Firefox 54 (master...firefox-54-compat)
19:59circleci-botFailed: ianb's build (#1116; push) in mozilla-services/screenshots (selenium-tests) --
20:04circleci-botFailed: ianb's build (#1120; push) in mozilla-services/screenshots (firefox-54-compat) --
20:05clouserwrelud: is a 403 for me
20:05reludclouserw: i will add you to the access list
20:14GitHub[screenshots] ianb commented on issue #2713: I think the build failure is simply due to Screenshots being built into Nightly now and pref'd off
20:16GitHub[screenshots] ianb force-pushed selenium-tests from 67b9b52 to 4b9d14a:
20:16GitHubscreenshots/selenium-tests 4b9d14a Ian Bicking: Increase test timeout, plus debugging console.log
20:19GitHub[screenshots] ianb commented on issue #2709: After discussion, we're not going to worry about the migration/database compatibility stuff for now.
20:22circleci-botFailed: ianb's build (#1121; push) in mozilla-services/screenshots (selenium-tests) --
20:26GitHub[screenshots] ianb force-pushed selenium-tests from 4b9d14a to 9a78434:
20:26GitHubscreenshots/selenium-tests 9a78434 Ian Bicking: Increase test timeout, plus debugging console.log
20:27circleci-botFixed: ianb's build (#1122; retry by dannycoates) in mozilla-services/screenshots (shutdown-in-bootstrap) --
20:28circleci-botFixed: ianb's build (#1123; retry by dannycoates) in mozilla-services/screenshots (firefox-54-compat) --
20:32circleci-botFailed: ianb's build (#1124; push) in mozilla-services/screenshots (selenium-tests) --
20:42GitHub[screenshots] ianb force-pushed selenium-tests from 9a78434 to a42cee2:
20:42GitHubscreenshots/selenium-tests a42cee2 Ian Bicking: Increase test timeout, plus debugging console.log, and other debugging attempts
20:46GitHub[screenshots] 6a68 commented on issue #2712: @ianb This looks good. Sadly, I think we need to change the "if reason == APP_STARTUP" bit from the startup method--otherwise I think the pref observer will stop working when the addon is upgraded. Would you mind setting a flag when the observer is registered, then checking that flag, instead of the reason constant, inside startup? We always want exactly one listener.
20:47GitHub[screenshots] 6a68 pushed 1 new commit to master:
20:47GitHubscreenshots/master 2f92593 Ian Bicking: Add third argument to add/removeObserver, which is required in Firefox 54 (#2713)
20:47GitHub[screenshots] 6a68 deleted firefox-54-compat at 916af99:
20:48GitHub[screenshots] 6a68 commented on issue #2713: @ianb do we need to uplift this with the initial round of patches?
20:48circleci-botFailed: ianb's build (#1125; push) in mozilla-services/screenshots (selenium-tests) --
20:49GitHub[screenshots] ianb commented on issue #2713: No, this was a regression from @Standard8's eslint patches, which aren't part of what's in Nightly now
20:52GitHub[screenshots] ianb commented on issue #2712: @6a68 I'm not sure I understand. `if (reason === APP_STARTUP)` only protects `appStartupObserver.register()` (which seems to unregister itself). But it doesn't stop the rest of startup() from being called, including the pref observer. Unless I miss what you are thinking about here.
20:53circleci-botFixed: 6a68's build (#1126; push) in mozilla-services/screenshots (master) --
21:19reludclouserw: access should be granted to you now
21:19clouserwrelud: thanks
21:29GitHub[screenshots] 6a68 commented on issue #2712: @ianb Ah yeah, you're right! Sorry about that, merging
21:30GitHub[screenshots] 6a68 pushed 1 new commit to master:
21:30GitHubscreenshots/master 9a23339 Ian Bicking: Shutdown the embedded webExtension when bootstrap is asked to shutdown (#2712)
21:30GitHub[screenshots] 6a68 deleted shutdown-in-bootstrap at 238f0fc:
21:32reludclouserw: ianbicking fzzzy: access to raw logs via ssh has changed to include a bastion for pageshot & screenshots, see docs here for the change:
21:33reludclouserw: ^ that applies to testpilot too, to which only you have access
21:38_6a68rhelmer: hey, are there any gotchas with system addon upgrades that we should be aware of now, before screenshots initially ships? I was just starting at bootstrap.js trying to think of places where multiple calls to startup/shutdown, or failed calls to startup/shutdown, could break stuff
21:39rhelmer_6a68: hmm I can't think of anything particularly tricky... it should look like a normal upgrade. the main difference is that the built-in add-on doesn't get uninstalled, so if the update is removed (such as on app upgrade) the built-in one is used instead
21:39rhelmerwe can also push an update that causes all upgrades to be removed
21:39rhelmer_6a68: easy way to test this is to host an XML file at extensions.systemAddon.update.url
21:39_6a68At this line, it looks like we might accidentally register multiple observers but there's a webextension.started check inside handleStartup that seems like it'll avoid duplicate start() calls
21:40_6a68Thanks for the info! So we do have the option to remove a bad upgrade after it ships, that's good to know
21:40_6a68Are there automated tests in the tree for other system addons to verify behavior across upgrades? Is this something we should test, or just assume the system works?
21:41rhelmerif an update doesn't apply then it should get rolled back. if calling a bootstrap method fails it won't automatically remove itself or anything though, so failing is possible post-install
21:42rhelmer_6a68: if you want to test, the xml format for updates looks like
21:42rhelmeryou can manually trigger the background update check with `AddonManagerPrivate.backgroundUpdateCheck()`
21:42rhelmerdocs are at
21:42rhelmerthat's checked into m-c
21:43rhelmer_6a68: oh yeah you know about the docs, you suggested those improvements at the top :)
21:43_6a68oh yeah! I've seen these docs before :-)
21:43_6a68Thanks for the info. I think we'll be okay, just trying to think of any last things as we move towards uplift
21:43rhelmer_6a68: anyway I'd suggest trying an update, then an update on top of that, etc.
21:43_6a68right, right
21:44rhelmerif you serve <updates><addons/></updates> that should cause it to remove all updates and fall back to built-in
21:44rhelmerwe have pretty extensive tests for this stuff but interaction with arbitrary code inside the addon always makes things interesting :)
21:45_6a68lol yes, and being the first webextension system addon makes this a little extra spicy
21:45rhelmerso that&#39;s the big switch we have to just reset everybody. unfortunately they only check once per 24h, but much easier than pushing a whole new build
21:49GitHub[screenshots] 6a68 opened issue #2714: Allow self-hosting via about:config pref?
21:56GitHub[screenshots] 6a68 opened issue #2715: Verify system addon upgrade works
22:04_6a68Working on a low-spec windows machine really gives you sympathy for the user
23:41GitHub[screenshots] 6a68 created disabled-button-visuals (+1 new commit):
23:41GitHubscreenshots/disabled-button-visuals b1415f6 Jared Hirsch: Fix #2708, manually dim toolbar button when disabled...
23:42GitHub[screenshots] 6a68 opened pull request #2716: Fix #2708, manually dim toolbar button when disabled (master...disabled-button-visuals)
23:48circleci-botFailed: 6a68&#39;s build (#1130; push) in mozilla-services/screenshots (disabled-button-visuals) --;utm_medium=referral&amp;utm_source=irc
20 Apr 2017
No messages
Last message: 157 days and 16 hours ago