Closed Bug 1419148 Opened 6 years ago Closed 6 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: 6 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.