Closed Bug 1368146 Opened 3 years ago Closed 3 years ago

Update Screenshots to version 8.1.0

Categories

(Firefox :: Screenshots, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Tracking Status
firefox55 --- fixed

People

(Reporter: _6a68, Assigned: _6a68)

References

Details

Attachments

(1 file)

Attachment #8871883 - Flags: feedback?(ianb)
Comment on attachment 8871883 [details]
Bug 1368146 - Export Screenshots 8.1.0 to Firefox;

https://reviewboard.mozilla.org/r/143402/#review147118
Attachment #8871883 - Flags: review+
Comment on attachment 8871883 [details]
Bug 1368146 - Export Screenshots 8.1.0 to Firefox;

https://reviewboard.mozilla.org/r/143402/#review147918

::: browser/extensions/screenshots/moz.build:18
(Diff revision 1)
>  # AUTOMATIC INSERTION START
>  FINAL_TARGET_FILES.features['screenshots@mozilla.org']["webextension"] += [
> +  'webextension/.eslintrc.js',
>    'webextension/assertIsTrusted.js',
>    'webextension/blank.html',
>    'webextension/buildSettings.js.template',

This looks wrong. We want buildSettings.js, not the template file.
Comment on attachment 8871883 [details]
Bug 1368146 - Export Screenshots 8.1.0 to Firefox;

https://reviewboard.mozilla.org/r/143402/#review147782

Looks reasonable. I've put a few notes below. I'd like the .eslintrc.js shipping issue fixed before this ships. The other issues I think we can file bugs on, but we must work out the regressing code (i.e. yield -> await / picking up block changes landing in the tree) before the next version of screenshots ships.

::: browser/extensions/screenshots/bootstrap.js:48
(Diff revision 1)
>    }
>  };
>  
> +const appStartupObserver = {
> +  register() {
> +    Services.obs.addObserver(this, "sessionstore-windows-restored", false); // eslint-disable-line mozilla/no-useless-parameters

I've a feeling this is going to mess with (mochi)tests when we try to enable screenshots for tests again, however, lets get this updated, and then work that out...

::: browser/extensions/screenshots/moz.build:15
(Diff revision 1)
>  ]
>  
>  # This file list is automatically generated by Screenshots' export scripts.
>  # AUTOMATIC INSERTION START
>  FINAL_TARGET_FILES.features['screenshots@mozilla.org']["webextension"] += [
> +  'webextension/.eslintrc.js',

We shouldn't be trying to ship this file.

::: browser/extensions/screenshots/test/browser/browser_screenshots_ui_check.js:10
(Diff revision 1)
>      is(!!document.getElementById(id), expectPresent, "element " + id + (expectPresent ? " is" : " is not") + " present");
>    }
>  }
>  
> -add_task(async function() {
> -  await promiseScreenshotsEnabled();
> +add_task(function*() {
> +  yield promiseScreenshotsEnabled();

This is worrying, if we're regressing the latest changes from the tree - this needs to be re-imported into the github repo.
Attachment #8871883 - Flags: review?(standard8) → review+
Comment on attachment 8871883 [details]
Bug 1368146 - Export Screenshots 8.1.0 to Firefox;

https://reviewboard.mozilla.org/r/143402/#review147782

> We shouldn't be trying to ship this file.

Fixed by 8.1.0 changes

> This is worrying, if we're regressing the latest changes from the tree - this needs to be re-imported into the github repo.

Cool. We'll fix this later in https://github.com/mozilla-services/screenshots/issues/2917
Pushed by mbanner@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e0e0d7d31cd5
Export Screenshots 8.1.0 to Firefox; r=ianb,standard8
https://hg.mozilla.org/mozilla-central/rev/e0e0d7d31cd5
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
== Change summary for alert #6946 (as of May 30 2017 19:29 UTC) ==

Improvements:

 11%  ts_paint_webext linux64 opt e10s     1,625.25 -> 1,438.58

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=6946
Assignee: nobody → jhirsch
Summary: Update Screenshots to version 8.0.0 → Update Screenshots to version 8.1.0
No longer blocks: 1372310
Product: Cloud Services → Firefox
You need to log in before you can comment on or make changes to this bug.