Closed Bug 1419148 Opened 8 years ago Closed 8 years ago

Export Screenshots 25.0.0

Categories

(Firefox :: Screenshots, defect)

Product:
Firefox
Component:
Screenshots
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 59
Tracking Flags:
Tracking Status
firefox58 --- fixed
firefox59 --- fixed

People

(Reporter: jhirsch, Assigned: jhirsch)

References

Details

Attachments

(2 files)

Bug 1419148 - Export Screenshots 25.0.0 to Firefox (translations only);
59 bytes, text/x-review-board-request
ianbicking
: review+
Details
Bug 1419148 - Export Screenshots 25.0.0 to Firefox (except translations);
59 bytes, text/x-review-board-request
kmag
: review+
ianbicking
: review+
gchang
: approval-mozilla-beta+
Details
An export of Screenshots 25.0.0. Changelog here: https://github.com/mozilla-services/screenshots/blob/e2847ed135acf7feda1de74c2241d00615428a26/CHANGELOG.md Items in this release: * Track the number of copied shots in Telemetry https://github.com/mozilla-services/screenshots/issues/3812 https://github.com/mozilla-services/screenshots/commit/336f147 * Add titles to preview overlay buttons. https://github.com/mozilla-services/screenshots/issues/3793 https://github.com/mozilla-services/screenshots/commit/d46985c * Add paste symbol for image copy notification. https://github.com/mozilla-services/screenshots/issues/3790 https://github.com/mozilla-services/screenshots/commit/5441349 * Add copy.svg path to manifest template https://github.com/mozilla-services/screenshots/issues/3786 https://github.com/mozilla-services/screenshots/commit/a6fa8c6 * Copy shot to clipboard. https://github.com/mozilla-services/screenshots/issues/2582 https://github.com/mozilla-services/screenshots/commit/58237e2 * Fix remaining domain regexes https://github.com/mozilla-services/screenshots/issues/3783 https://github.com/mozilla-services/screenshots/commit/5089187 * Update CSS to fix notice tooltip. https://github.com/mozilla-services/screenshots/issues/3780 https://github.com/mozilla-services/screenshots/commit/78e5853 * instrument response times in shot creation flow https://github.com/mozilla-services/screenshots/issues/3673 https://github.com/mozilla-services/screenshots/commit/82cb4b7 * Remove unnecessary icon path for page action. https://github.com/mozilla-services/screenshots/issues/3760 https://github.com/mozilla-services/screenshots/commit/bc29b56 * use content.XMLHttpRequest instead of wrappedJSObject.XMLHttpRequest https://github.com/mozilla-services/screenshots/issues/3626 https://github.com/mozilla-services/screenshots/commit/6cb98f6 * Use generic notice css class. https://github.com/mozilla-services/screenshots/issues/3701 https://github.com/mozilla-services/screenshots/commit/927f39a * Stop using window.inner[Width|Height] b/c scroll bars. https://github.com/mozilla-services/screenshots/issues/3641 https://github.com/mozilla-services/screenshots/commit/647819e * Remove onboarding icon. https://github.com/mozilla-services/screenshots/issues/3542 https://github.com/mozilla-services/screenshots/commit/00f0409 * Use dl-only notice style for cropped img warning. https://github.com/mozilla-services/screenshots/issues/3701 https://github.com/mozilla-services/screenshots/commit/60a5b2f * Move start time for error timer into startBackground. https://github.com/mozilla-services/screenshots/issues/3707 https://github.com/mozilla-services/screenshots/commit/5a3e01c * Properly set the Photon page action's title and icon. Bug 1395387 changed the Photon page action API so that `title` and `iconURL` are no longer properties but methods, setTitle() and setIconURL(). We need to update our consumer. Right now, setting these properties isn't doing anything at all. https://github.com/mozilla-services/screenshots/commit/2e15e64 * Add left margin to selection frame to handle doc width < viewport width https://github.com/mozilla-services/screenshots/issues/3256 https://github.com/mozilla-services/screenshots/commit/49bafca * Download shot when Enter is pressed in download only mode. https://github.com/mozilla-services/screenshots/issues/3714 https://github.com/mozilla-services/screenshots/commit/df973cd * Opt out of webdriver click in Selenium tests. https://github.com/mozilla-services/screenshots/issues/3734 https://github.com/mozilla-services/screenshots/commit/c2ea6b3 * Bump pixel limit to 10000. https://github.com/mozilla-services/screenshots/issues/3668 https://github.com/mozilla-services/screenshots/commit/328696f * Remove extra preselection frame unhide() (that's before frame's loaded). https://github.com/mozilla-services/screenshots/issues/3729 https://github.com/mozilla-services/screenshots/commit/263c990 * send a response after incrementing counts https://github.com/mozilla-services/screenshots/issues/3718 https://github.com/mozilla-services/screenshots/commit/b40f1dd * style tweaks for download only UI https://github.com/mozilla-services/screenshots/issues/3705 https://github.com/mozilla-services/screenshots/commit/b40841d * Fixed http/https regex checks https://github.com/mozilla-services/screenshots/pull/3720 https://github.com/mozilla-services/screenshots/commit/96f5fc4 * add Telemetry scalars to track shot creation per session. For related, Gecko-only changes to the telemetry Scalars.yaml file, see https://bugzilla.mozilla.org/show_bug.cgi?id=1412411. https://github.com/mozilla-services/screenshots/issues/2218 https://github.com/mozilla-services/screenshots/commit/80ecdd0 * disable full-page truncation warning if download-only warning will be shown. https://github.com/mozilla-services/screenshots/issues/3700 https://github.com/mozilla-services/screenshots/commit/e3ee5af * fix rtl save icon alignment https://github.com/mozilla-services/screenshots/issues/3514 https://github.com/mozilla-services/screenshots/commit/d37a4ff * Use download only mode if 'never remember history' is checked. https://github.com/mozilla-services/screenshots/issues/3574 https://github.com/mozilla-services/screenshots/commit/d0d4db3 https://github.com/mozilla-services/screenshots/commit/1c28bbf * Download only mode: enable Screenshots in private browsing mode, use the private browsing download manager, and create download only mode for private browsing https://github.com/mozilla-services/screenshots/issues/3655 https://github.com/mozilla-services/screenshots/commit/1223743
Here's a try push with 5 Talos retries, just to be sure we aren't going to impact performance: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d44761f2508322b35752e150ef3103e838e22002
Assignee: nobody → jhirsch
Comment on attachment 8930197 [details] Bug 1419148 - Export Screenshots 25.0.0 to Firefox (translations only); https://reviewboard.mozilla.org/r/201350/#review206474
Attachment #8930197 - Flags: review?(ianb) → review+
Comment on attachment 8930198 [details] Bug 1419148 - Export Screenshots 25.0.0 to Firefox (except translations); https://reviewboard.mozilla.org/r/201352/#review206480 ::: browser/extensions/screenshots/webextension/selector/ui.js:408 (Diff revision 3) > <body> > <div class="preview-overlay"> > <div class="preview-image"> > <div class="preview-buttons"> > - <button class="highlight-button-cancel"></button> > - <button class="highlight-button-download"></button> > + <button class="highlight-button-cancel" > + title="${browser.i18n.getMessage("cancelScreenshot")}"></button> At the moment we don't have any localizations that will break this, but we could. I made a followup in [#3815](https://github.com/mozilla-services/screenshots/issues/3815)
Attachment #8930198 - Flags: review?(ianb) → review+
I added two fixes not mentioned in the changelog above: * Escape quotes in localized attributes (addresses comment 9 in this bug) https://github.com/mozilla-services/screenshots/issues/3815 https://github.com/mozilla-services/screenshots/commit/808e65b * Silence any timing-related errors https://github.com/mozilla-services/screenshots/commit/49bf56e
Comment on attachment 8930198 [details] Bug 1419148 - Export Screenshots 25.0.0 to Firefox (except translations); https://reviewboard.mozilla.org/r/201352/#review207510 ::: browser/extensions/screenshots/bootstrap.js:227 (Diff revision 4) > if (addon) { > addon.uninstall(); > } > sendReply({type: "success", value: !!addon}); > }); > return true; Hm. You should be using promises for this, rather than `sendReply`, especially when you're sending the reply asynchronously... ::: browser/extensions/screenshots/bootstrap.js:231 (Diff revision 4) > + } else if (msg.funcName === "incrementDownloadCount") { > + Services.telemetry.scalarAdd('screenshots.download', 1); Any particular reason to have a separate message for each of these scalars, rather than just sending the scalar name from the add-on side? ::: browser/extensions/screenshots/webextension/background/analytics.js:31 (Diff revision 4) > + req.send(JSON.stringify({ > + deviceId: auth.getDeviceId(), > + timingCategory, > + timingLabel, > + timingVar, > + timingValue > + })); I'm assuming you already have data review for this? ::: browser/extensions/screenshots/webextension/background/analytics.js:116 (Diff revision 4) > exports.getTelemetryPrefSync = function() { > catcher.watchPromise(exports.refreshTelemetryPref()); > return !!telemetryPref; > }; > > + let timingData = {}; Nit: This should be a `Map`. ::: browser/extensions/screenshots/webextension/background/analytics.js:200 (Diff revision 4) > + filter.action === action && filter.label === label : > + filter.action === action; > + } > + > + function anyMatches(filters, action, label) { > + return !!filters.find(filter => match(filter, action, label)); Nit: `return filters.some(...)` ::: browser/extensions/screenshots/webextension/background/analytics.js:208 (Diff revision 4) > + function measureTiming(action, label) { > + rules.forEach(r => { > + if (anyMatches(r.cancel, action, label)) { > + delete timingData[r.name]; > + } else if (match(r.start, action, label)) { > + timingData[r.name] = Date.now(); Nit: You should probably be using `performance.now()` ::: browser/extensions/screenshots/webextension/background/main.js:242 (Diff revision 4) > }); > browser.downloads.onChanged.addListener(onChangedCallback) > + catcher.watchPromise(communication.sendToBootstrap("incrementDownloadCount")); > + return browser.windows.getLastFocused().then(windowInfo => { > + return windowInfo.incognito; > + }).then((incognito) => { Why do you need a separate `.then()` clause for this? ::: browser/extensions/screenshots/webextension/selector/ui.js:385 (Diff revision 4) > } > }; > > + function getAttributeText(l10nID) { > + let text = browser.i18n.getMessage(l10nID); > + return text && text.replace('"', "&quot;"); Not really good enough. Please escape all of: <>"&
Attachment #8930198 - Flags: review?(kmaglione+bmo) → review+
Waiting for data review before landing updated patch: https://github.com/mozilla-services/screenshots/issues/3839 Kicked off a Try run in the meantime, to be sure this patch doesn't regress Talos: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5696688afd8f
Got data review r+, landing. I'll keep an eye on Talos.
Comment on attachment 8930198 [details] Bug 1419148 - Export Screenshots 25.0.0 to Firefox (except translations); https://reviewboard.mozilla.org/r/201352/#review208662 ::: browser/extensions/screenshots/bootstrap.js:227 (Diff revision 4) > if (addon) { > addon.uninstall(); > } > sendReply({type: "success", value: !!addon}); > }); > return true; I actually don't think we need this code any more, since we've migrated all the page shot test pilot experiment users and have shut down the page shot service. Issue opened on Github to track removal of this and related code: https://github.com/mozilla-services/screenshots/issues/3844 ::: browser/extensions/screenshots/webextension/background/analytics.js:208 (Diff revision 4) > + function measureTiming(action, label) { > + rules.forEach(r => { > + if (anyMatches(r.cancel, action, label)) { > + delete timingData[r.name]; > + } else if (match(r.start, action, label)) { > + timingData[r.name] = Date.now(); I considered that, but the latencies for some steps in the shot flow are on the order of whole seconds, so millisecond precision is good enough (for now).
Comment on attachment 8930198 [details] Bug 1419148 - Export Screenshots 25.0.0 to Firefox (except translations); https://reviewboard.mozilla.org/r/201352/#review207510 > I'm assuming you already have data review for this? Yes, https://github.com/mozilla-services/screenshots/issues/3839
Pushed by jhirsch@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/2b5e9ba6041f Export Screenshots 25.0.0 to Firefox (translations only); r=ianbicking https://hg.mozilla.org/integration/autoland/rev/d1535dbb8e80 Export Screenshots 25.0.0 to Firefox (except translations); r=ianbicking,kmag
https://hg.mozilla.org/mozilla-central/rev/2b5e9ba6041f https://hg.mozilla.org/mozilla-central/rev/d1535dbb8e80
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox59: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 59
Comment on attachment 8930198 [details] Bug 1419148 - Export Screenshots 25.0.0 to Firefox (except translations); Approval Request Comment [Feature/Bug causing the regression]: Import recent features & bug fixes from github (see changelog in the description of this bug and bug 1419148) [User impact if declined]: [Is this code covered by automated tests?]: No [Has the fix been verified in Nightly?]: Yes [Needs manual test from QE? If yes, steps to reproduce]: SV has tested already against Nightly, will test against Beta [List of other uplifts needed for the feature/fix]: The other commit on this bug, and the 3 commits on bug 1412091 [Is the change risky?]: Low risk [Why is the change risky/not risky?]: Changes are confined to the webextension part of the Screenshots system add-on, have been tested by SV, and have spent time in Nightly without new issues. [String changes made/needed]: See separate l10n commit on this same bug
Attachment #8930198 - Flags: approval-mozilla-beta?
Comment on attachment 8930198 [details] Bug 1419148 - Export Screenshots 25.0.0 to Firefox (except translations); For better user experiences, take this in Beta58. Beta58+.
Attachment #8930198 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: