remove leftover code and strings from standalone about:addons UI

VERIFIED FIXED in Firefox 58

Status

()

enhancement
P5
normal
VERIFIED FIXED
2 years ago
a year ago

People

(Reporter: rhelmer, Assigned: sajattack, Mentored)

Tracking

(Depends on 1 bug, {good-first-bug})

unspecified
mozilla58
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox58 fixed)

Details

(Whiteboard: triaged )

Attachments

(1 attachment, 1 obsolete attachment)

Reporter

Description

2 years ago
Other than the tests in bug 1349723, there is some leftover code (nav buttons) and related strings that should be removed.
Reporter

Updated

2 years ago
Depends on: 1349723

Updated

2 years ago
Priority: -- → P5
Whiteboard: triaged
Reporter

Updated

2 years ago
Keywords: good-first-bug
Whiteboard: triaged → triaged
Rob, are you still working on this or should we open it up to contributors?
Flags: needinfo?(rhelmer)
Reporter

Comment 2

2 years ago
(In reply to Andreas Wagner [:TheOne] [use NI] from comment #1)
> Rob, are you still working on this or should we open it up to contributors?

Nope meant to unassign, thanks!
Assignee: rhelmer → nobody
Flags: needinfo?(rhelmer)
Mentor: rhelmer
Assignee

Comment 3

2 years ago
What file is the leftover code in?
sajattack, when you have a question like this, please use the "need more information from" button at the bottom of the page to make sure the person you're directing the question to realizes that you're waiting for a reply.  In this case, rhelmer is the bug mentor.
Flags: needinfo?(rhelmer)

Comment 5

2 years ago
Hi, can i work on this bug?
Reporter

Comment 6

2 years ago
(In reply to sajattack from comment #3)
> What file is the leftover code in?

The leftover code is in extensions.xul - I'd suggest trying to remove the "back-btn" and "forward-btn" toolbarbuttons in http://searchfox.org/mozilla-central/rev/e76c0fee79a34a3612740b33508276f4b37c3ee8/toolkit/mozapps/extensions/content/extensions.xul#138 and seeing which tests fail.

Also, any strings used (such as "cmd.back.tooltip") should be removed as well. These are in the extensions.dtd http://searchfox.org/mozilla-central/rev/e76c0fee79a34a3612740b33508276f4b37c3ee8/toolkit/locales/en-US/chrome/mozapps/extensions/extensions.dtd#26

Thanks for working on this!
Flags: needinfo?(rhelmer)
Assignee

Comment 7

2 years ago
The tests take quite some time to run on my machine. Could you give me access to the try server?
Flags: needinfo?(rhelmer)
Reporter

Comment 8

2 years ago
(In reply to sajattack from comment #7)
> The tests take quite some time to run on my machine. Could you give me
> access to the try server?

I don't have the ability to do this unfortunately, but I feel your pain.. if you post a patch I'd be happy to run it on the try server for you!
Flags: needinfo?(rhelmer)
Assignee

Comment 9

2 years ago
My test suite finally finished. Everything looks good except for a few fails that seem unrelated.
Comment hidden (mozreview-request)
Assignee

Updated

2 years ago
Flags: needinfo?(rhelmer)
Assignee

Updated

2 years ago
Flags: needinfo?(rhelmer)
Reporter

Comment 13

2 years ago
(In reply to Robert Helmer [:rhelmer] from comment #12)
> I went ahead and pushed from mozreview:
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=ee3b8763e672d450d6ebe2dbf15739456e9da792

Hm this looks like it might be a real test fail - are you able to reproduce this one locally?
https://treeherder.mozilla.org/logviewer.html#?job_id=132054523&repo=try

Thanks!
Flags: needinfo?(sajattack)
Reporter

Comment 14

2 years ago
Looks like the problem is here:

http://searchfox.org/mozilla-central/rev/2ef8bd8a46a02c68ddbb1d5f25fa254dd7be1fbd/toolkit/mozapps/extensions/content/extensions.js#2096-2100

I'd suggest searching for the IDs being removed here (back-btn, forward-btn) to see if there are any more that could be removed:
http://searchfox.org/mozilla-central/search?q=back-btn
Reporter

Updated

2 years ago
Assignee: nobody → sajattack
Reporter

Updated

2 years ago
Attachment #8908359 - Attachment is obsolete: true
Reporter

Comment 15

2 years ago
mozreview-review
Comment on attachment 8908362 [details]
Bug 1364333 - remove leftover code and strings from standalone about:addons UI

https://reviewboard.mozilla.org/r/179982/#review187604

Looks good, per comment 14 there are a few more places to change.
Attachment #8908362 - Flags: review?(rhelmer)
Comment hidden (mozreview-request)
Reporter

Comment 17

2 years ago
Doing another try run... pretty sure I kicked one off yesterday but not seeing it attached to mozreview.

In any case, I see a minor ESlint problem so far (variable declared but unused), let's see if the tests pass now:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c0d0e276475e389aae31722e8c7b3f64c4c70930
Reporter

Comment 18

2 years ago
mozreview-review
Comment on attachment 8908362 [details]
Bug 1364333 - remove leftover code and strings from standalone about:addons UI

https://reviewboard.mozilla.org/r/179982/#review189334

Looks great! There's a small issue that ESLint found, would you mind pushing a fix for this and I'll merge? Thanks for your contribution!
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c0d0e276475e389aae31722e8c7b3f64c4c70930&selectedJob=133610461
Attachment #8908362 - Flags: review?(rhelmer) → review+
Comment hidden (mozreview-request)
Assignee

Updated

2 years ago
Flags: needinfo?(sajattack)

Comment 20

2 years ago
Pushed by rhelmer@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f0d6d70e39ae
remove leftover code and strings from standalone about:addons UI r=rhelmer

Comment 21

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/f0d6d70e39ae
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Thanks so much for the patch, sajattack! Your contribution has been added to our recognition wiki: https://wiki.mozilla.org/Add-ons/Contribute/Recognition#September_2017

If you'd like to set up a profile on mozillians.org, I'd be happy to vouch for you. 

Welcome onboard! I look forward to seeing you around.
Assignee

Comment 23

2 years ago
Wow. It's an honour Caitlin.
Fabulous! You are vouched. :)
Depends on: 1410175
This regressed the top padding on the categories since there was an override to account for the added height from this component. That is tracked in bug 1410175.

Comment 27

2 years ago
Verified in FF58(Win7) I could not find any other regressions around about:addons. Whould any other kind of manual testing be necessary in here or we can mark this bug as verified fixed and track the padding issue in bug 141074?
Flags: needinfo?(rhelmer)
Reporter

Comment 28

a year ago
(In reply to Madalin Cotetiu from comment #27)
> Verified in FF58(Win7) I could not find any other regressions around
> about:addons. Whould any other kind of manual testing be necessary in here
> or we can mark this bug as verified fixed and track the padding issue in bug
> 141074?


Sorry for the delay - I don't think there's anything left to do here. Thanks!
Status: RESOLVED → VERIFIED
Flags: needinfo?(rhelmer)
You need to log in before you can comment on or make changes to this bug.