Closed Bug 1364333 Opened 4 years ago Closed 4 years ago

remove leftover code and strings from standalone about:addons UI

Categories

(Toolkit :: Add-ons Manager, enhancement, P5)

Product:
Toolkit
Component:
Add-ons Manager
Type:
enhancement

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla58
Tracking Flags:
Tracking Status
firefox58 --- fixed

People

(Reporter: rhelmer, Assigned: sajattack, Mentored)

References

Details

(Keywords: good-first-bug, Whiteboard: triaged )

Attachments

(1 file, 1 obsolete file)

Bug 1364333 - remove leftover code and strings from standalone about:addons UI
59 bytes, text/x-review-board-request
rhelmer
: review+
Details 
Other than the tests in bug 1349723, there is some leftover code (nav buttons) and related strings that should be removed.
Depends on: 1349723
Priority: -- → P5
Whiteboard: triaged
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)
(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
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)
Hi, can i work on this bug?
(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)
The tests take quite some time to run on my machine. Could you give me access to the try server?
Flags: needinfo?(rhelmer)
(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)
My test suite finally finished. Everything looks good except for a few fails that seem unrelated.
Flags: needinfo?(rhelmer)
Flags: needinfo?(rhelmer)
(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)
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
Assignee: nobody → sajattack
Attachment #8908359 - Attachment is obsolete: true
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)
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
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+
Flags: needinfo?(sajattack)
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
https://hg.mozilla.org/mozilla-central/rev/f0d6d70e39ae
Status: NEW → RESOLVED
Closed: 4 years ago
status-firefox58: --- → fixed
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.
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.
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)
(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.