Export Screenshots 23.0.0

RESOLVED FIXED in Firefox 58

Status

()

defect
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: ianbicking, Assigned: ianbicking)

Tracking

unspecified
Firefox 59
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox58 fixed, firefox59 fixed)

Details

Attachments

(3 attachments)

An export of Screenshots 23.0.0. Changelog here: https://github.com/mozilla-services/screenshots/blob/ab0908c94990e439244aafc8b6313c4e5b22f98d/CHANGELOG.md#add-on-changes

Items in this release:

* Get doc width and height from function in case of resize. https://github.com/mozilla-services/screenshots/issues/3506 (https://github.com/mozilla-services/screenshots/commit/cd4e777)

* Allow full size downloaded shots. https://github.com/mozilla-services/screenshots/issues/3506 (https://github.com/mozilla-services/screenshots/commit/4c6f9d5)

* Fix direction of rtl-language arrows in onboarding (https://github.com/mozilla-services/screenshots/commit/ef0508c)

* Fix Selenium test https://github.com/mozilla-services/screenshots/issues/3647
  * This adds a timeout after the UI is first displayed. The UI is displayed, and handlers are added, but for EVERYTHING to get setup and working takes slightly longer. Fixes https://github.com/mozilla-services/screenshots/issues/3616 (https://github.com/mozilla-services/screenshots/commit/be2284a)

* Load scripts at document_start. https://github.com/mozilla-services/screenshots/issues/3633 (https://github.com/mozilla-services/screenshots/commit/6e4f27d)

* Avoid infinite selection loop by checking minimum sizd. Source code files on dxr.mozilla.org are rendered as floats inside a non-clearfixed parent element, which has a calculated height of 0. This condition isn't handled in the current loop logic, leading to an infinite while loop that locks up the UI. Fixes https://github.com/mozilla-services/screenshots/issues/3314 (https://github.com/mozilla-services/screenshots/commit/7c0be97)

* Use JS to detect and apply high contrast mode styles. Fixes https://github.com/mozilla-services/screenshots/issues/3174 Fixes https://github.com/mozilla-services/screenshots/issues/3565 (https://github.com/mozilla-services/screenshots/commit/8bcf729)

* Notify user when full page is cut off. Adds a new captureType, fullPageTruncated. Fixes https://github.com/mozilla-services/screenshots/issues/2129 (https://github.com/mozilla-services/screenshots/commit/aa97577)

* Put limits on uses of string.split. This only covers cases in the JPEG commits, to keep the resulting diff minimal (https://github.com/mozilla-services/screenshots/commit/e9763fc)

* Fix an undefined variable (bad rename) in the Mochitest (https://github.com/mozilla-services/screenshots/commit/25aeaa1)

A few of these were cherry-picked into Firefox already.
Attachment #8922917 - Flags: review?(jhirsch)
Attachment #8922918 - Flags: review?(jhirsch)
Attachment #8922919 - Flags: review?(jhirsch)
I dunno if we should be self-reviewing exports into Firefox quite yet. :mossop or :kmag, thoughts?
Flags: needinfo?(kmaglione+bmo)
Flags: needinfo?(dtownsend)
I was planning on bringing it up after QA got through it, but yeah, I'm not sure about the review process. Nevertheless, I thought Jared could review the export for correctness/sanity.
An initial review from a member of your team is always welcome. I'm happy for translation-only changes to land without additional review.

For changes to chrome-privileged code and security-sensitive content script code, I'd still like an additional sign-off from a Firefox peer.

For changes to screenshots UI code that isn't part of a content script, please use your judgment.
Flags: needinfo?(kmaglione+bmo)
Deferring to Kris here.
Flags: needinfo?(dtownsend)
Attachment #8922919 - Flags: review?(kmaglione+bmo)
Comment on attachment 8922917 [details]
Bug 1412091 - Export Screenshots 23.0.0 to Firefox (translations only)

https://reviewboard.mozilla.org/r/194092/#review202448
Attachment #8922917 - Flags: review?(jhirsch) → review+
Comment on attachment 8922918 [details]
Bug 1412091 - Export Screenshots 23.0.0 to Firefox (update raven to 3.18.1)

https://reviewboard.mozilla.org/r/194094/#review202452
Attachment #8922918 - Flags: review?(jhirsch) → review+
Comment on attachment 8922919 [details]
Bug 1412091 - Export Screenshots 23.0.0 to Firefox (all except translations and vendor library update)

https://reviewboard.mozilla.org/r/194096/#review202456

Export looks good to me. Individual patches have already been reviewed on Github; see changelog for links to pull requests.
Attachment #8922919 - Flags: review?(jhirsch) → review+
Comment on attachment 8922919 [details]
Bug 1412091 - Export Screenshots 23.0.0 to Firefox (all except translations and vendor library update)

Would you mind taking a quick look at this commit?
Flags: needinfo?(kmaglione+bmo)
Comment on attachment 8922919 [details]
Bug 1412091 - Export Screenshots 23.0.0 to Firefox (all except translations and vendor library update)

https://reviewboard.mozilla.org/r/194096/#review202574

Sorry, I hope you weren't waiting on my review. I was waiting on Jared's initial review.

::: browser/extensions/screenshots/moz.build:60
(Diff revision 1)
> +FINAL_TARGET_FILES.features['screenshots@mozilla.org']["webextension"]["_locales"]["bs"] += [
> +  'webextension/_locales/bs/messages.json'
> +]

In the future, can you try to keep these changes in the translations commit?

::: browser/extensions/screenshots/webextension/background/takeshot.js:175
(Diff revision 1)
>          return {
>            "content-type": "application/json",
>            body: JSON.stringify(shot.asJson())
>          };
> -      }
> +

Nit: Please fix indentation.

::: browser/extensions/screenshots/webextension/sitehelper.js:11
(Diff revision 1)
>  
>  this.sitehelper = (function() {
>  
> +  // This gives us the content's copy of XMLHttpRequest, instead of the wrapped
> +  // copy that this content script gets:
> +  let ContentXMLHttpRequest = window.wrappedJSObject.XMLHttpRequest;

This isn't safe... But I suppose it's not a huge issue as long as it's only being used on screenshots.firefox.com.

Please add an assertion that the TLD is correct, though. This makes me uncomfortable.
Attachment #8922919 - Flags: review?(kmaglione+bmo) → review+
Flags: needinfo?(kmaglione+bmo)
> This isn't safe... But I suppose it's not a huge issue as long as it's only being used on screenshots.firefox.com.
> 
> Please add an assertion that the TLD is correct, though. This makes me uncomfortable.

Can't we be sure of the page because of the content_script? https://github.com/mozilla-services/screenshots/blob/master/addon/webextension/manifest.json.template#L23 (note this is set to http://localhost:10080 in development, and https://screenshots.firefox.com in production – and at the moment sitehelper.js doesn't have a copy of the expected value, so there's no one value it can assert)

We also have a followup bug to remove this: https://github.com/mozilla-services/screenshots/issues/3626 – this patch preceded the WebExtension change in Bug 1295660
Flags: needinfo?(kmaglione+bmo)
Pinged kmag on IRC, his feedback: "I'd rather have the assertion. I don't care if you also allow localhost. The content script include patterns *should* be enough, but it's too far removed from that code, which makes me uncomfortable"
Flags: needinfo?(kmaglione+bmo) → needinfo?(ianb)
I've put in the assertion. I haven't made an equivalent upstream change, so it'll get reverted in the next push. I'm just going to get https://github.com/mozilla-services/screenshots/issues/3626 in before the next push, which will make it moot.
Flags: needinfo?(ianb)
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/8064694cdab8
Export Screenshots 23.0.0 to Firefox (translations only) r=_6a68
https://hg.mozilla.org/integration/autoland/rev/dbe32f48da6f
Export Screenshots 23.0.0 to Firefox (update raven to 3.18.1) r=_6a68
https://hg.mozilla.org/integration/autoland/rev/2ce1806a42ca
Export Screenshots 23.0.0 to Firefox (all except translations and vendor library update) r=kmag,_6a68
Keywords: checkin-needed
Comment on attachment 8922919 [details]
Bug 1412091 - Export Screenshots 23.0.0 to Firefox (all except translations and vendor library update)

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 2 commits on this bug, and the 2 commits on bug 1419148

[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 #8922919 - Flags: approval-mozilla-beta?
NI :flod for translation part.
Flags: needinfo?(francesco.lodolo)
(In reply to Gerry Chang [:gchang] from comment #22)
> NI :flod for translation part.

No impact on l10n, strings are translated outside of mozilla-central, and are already in use on Nightly for the testing part.
Flags: needinfo?(francesco.lodolo)
Comment on attachment 8922919 [details]
Bug 1412091 - Export Screenshots 23.0.0 to Firefox (all except translations and vendor library update)

Per discussion with clouserw and erin, we can take this for better user experiences. Beta58+.
Attachment #8922919 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.