Remove em:unpack from the mozscreenshots extension

RESOLVED FIXED in Firefox 58

Status

RESOLVED FIXED
a year ago
a year ago

People

(Reporter: MattN, Assigned: chochri5, Mentored)

Tracking

(Blocks: 1 bug)

unspecified
mozilla58
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox58 fixed)

Details

(URL)

Attachments

(1 attachment, 1 obsolete attachment)

Bug 1372696 is removing support for em:unpack and I'm not sure it's really needed for mozscreenshots from central anymore since we're currently using the screenshot binaries provided by automation (for better or worse).
Component: General → mozscreenshots
Product: Firefox → Testing
Version: Trunk → unspecified
Assignee: nobody → chochri5
Mentor: mconley, jaws
Status: NEW → ASSIGNED
Comment hidden (mozreview-request)
(Reporter)

Comment 2

a year ago
mozreview-review
Comment on attachment 8912080 [details]
Bug 1374827 - Remove lines dealing with em:unpack from mozscreenshots extension.

https://reviewboard.mozilla.org/r/183458/#review188626

::: browser/tools/mozscreenshots/mozscreenshots/extension/install.rdf:14
(Diff revision 1)
>  
>      <!-- for running custom screenshot binaries -->
> -    <em:unpack>true</em:unpack>
> +    <em:unpack>false</em:unpack>

Hi Chris, the default value is false when omitted so the tag should be deleted instead. You should also remove the comment above it since it's describing why `true` was necessary.
Comment hidden (mozreview-request)

Comment 4

a year ago
mozreview-review
Comment on attachment 8912331 [details]
Bug 1374827 - Removed lines dealing with em:unpack

https://reviewboard.mozilla.org/r/183666/#review188904

Thanks Chris!

This looks great to me, but can you try folding these two commits together?

You'll need to use the `histedit` Mercurial extension to do that. `histedit` should have been enabled when you ran `./mach mercurial-setup`.

Assuming you have no uncommitted changes, and you have the "Removed lines dealing with em:unpack" commit checked out, I think you should be able to do:

`hg histedit -r bc631c966688`

to bring up the vim interface for picking, dropping, re-ordering, and merging commits. This is very similar to `git rebase --interactive`. What you'll want to do is fold the second commit into the first by changing its first term from `pick` to `fold`. Once that's done, exit the editor (hopefully you're okay with exiting Vim - If not, ping me in IRC, and I'll walk you through it).

You'll probably then get Vim again, asking you to update the commit message for the now-merged commits. Maybe make it something like:

`Bug 1374827 - Remove lines dealing with em:unpack from mozscreenshots extension. r?mconley`.
Attachment #8912331 - Flags: review?(mconley) → review-

Comment 5

a year ago
mozreview-review
Comment on attachment 8912080 [details]
Bug 1374827 - Remove lines dealing with em:unpack from mozscreenshots extension.

https://reviewboard.mozilla.org/r/183458/#review188908
Attachment #8912080 - Flags: review?(mconley)
Comment hidden (mozreview-request)
(Assignee)

Updated

a year ago
Attachment #8912331 - Attachment is obsolete: true

Comment 7

a year ago
mozreview-review
Comment on attachment 8912080 [details]
Bug 1374827 - Remove lines dealing with em:unpack from mozscreenshots extension.

https://reviewboard.mozilla.org/r/183458/#review189226

::: commit-message-9db1c:1
(Diff revision 2)
> +Bug 1374827 - Change em:unpack value to 'false' r?mconley
> +
> +MozReview-Commit-ID: 7kSOhkx6idu
> +***
> +
> +Bug 1374827 - Remove lines dealing with em:unpack from mozscreenshots
> +extension. r?mconley
> +
> +MozReview-Commit-ID: FlEkeeIUNsL

Great! Are you able to do:

`hg commit --amend`

to update this commit message? It's folded two commit messages together.

I suspect we'll want the commit message to look like this:

```
Bug 1374827 - Remove lines dealing with em:unpack from mozscreenshots
extension. r?mconley
```
Attachment #8912080 - Flags: review?(mconley)

Comment 8

a year ago
mozreview-review-reply
Comment on attachment 8912080 [details]
Bug 1374827 - Remove lines dealing with em:unpack from mozscreenshots extension.

https://reviewboard.mozilla.org/r/183458/#review188626

> Hi Chris, the default value is false when omitted so the tag should be deleted instead. You should also remove the comment above it since it's describing why `true` was necessary.

Hey Chris, you'll also want to mark this issue as "Fixed", otherwise we can't autoland it.
Comment hidden (mozreview-request)

Comment 10

a year ago
mozreview-review
Comment on attachment 8912080 [details]
Bug 1374827 - Remove lines dealing with em:unpack from mozscreenshots extension.

https://reviewboard.mozilla.org/r/183458/#review189308

::: commit-message-9db1c:1
(Diff revision 3)
> +Bug 1374827 - Remove lines dealing with em:unpack from mozscreenshots
> +extension. r?mconley

These should be on a single line.

::: commit-message-9db1c:5
(Diff revision 3)
> +***
> +
> +Bug 1374827 - Remove lines dealing with em:unpack from mozscreenshots
> +extension. r?mconley
> +
> +MozReview-Commit-ID: FlEkeeIUNsL

Can you please remove these lines as well?
Attachment #8912080 - Flags: review?(mconley) → review-
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 13

a year ago
mozreview-review
Comment on attachment 8912080 [details]
Bug 1374827 - Remove lines dealing with em:unpack from mozscreenshots extension.

https://reviewboard.mozilla.org/r/183458/#review190236

Looks great, thanks!
Attachment #8912080 - Flags: review?(mconley) → review+

Comment 14

a year ago
Pushed by mconley@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/cecd6d07c8bf
Remove lines dealing with em:unpack from mozscreenshots extension. r=mconley
https://hg.mozilla.org/mozilla-central/rev/cecd6d07c8bf
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox58: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
You need to log in before you can comment on or make changes to this bug.