Closed
Bug 599771
Opened 14 years ago
Closed 14 years ago
Update buttons in the Add-ons Manager to use a consistent id/anonid naming scheme
Categories
(Toolkit :: Add-ons Manager, defect)
Toolkit
Add-ons Manager
Tracking
()
VERIFIED
FIXED
mozilla2.0b7
People
(Reporter: whimboo, Assigned: whimboo)
References
Details
(Whiteboard: [mozmill-needed])
Attachments
(1 file, 6 obsolete files)
83.04 KB,
patch
|
mossop
:
review+
mossop
:
approval2.0+
|
Details | Diff | Splinter Review |
The restart button link doesn't have any anonid accociated to it. That makes it hard to easily find this button. An anonid should be added, similar to all the other buttons. We need this fix to let Mozmill handle this button link.
Assignee | ||
Comment 1•14 years ago
|
||
Comment on attachment 478680 [details] [diff] [review] Patch v1 There are some more missing anonids around.
Attachment #478680 -
Attachment is obsolete: true
Attachment #478680 -
Flags: review?(dtownsend)
Assignee | ||
Updated•14 years ago
|
Summary: Restart button links are missing id or anonid → Restart and more button-link's are missing id or anonid
Assignee | ||
Comment 2•14 years ago
|
||
I hope it's ok to move out the details class into the anonid. I wasn't able to find any other code and test which relies on it. It will make it consistent to all other button links.
Attachment #478682 -
Flags: review?(dtownsend)
Comment 3•14 years ago
|
||
Comment on attachment 478682 [details] [diff] [review] Patch v1.1 The details class is used in the theme, please leave it there
Attachment #478682 -
Flags: review?(dtownsend) → review-
Assignee | ||
Comment 4•14 years ago
|
||
(In reply to comment #3) > Comment on attachment 478682 [details] [diff] [review] > Patch v1.1 > > The details class is used in the theme, please leave it there Is there any reason why we need the details class? All other button-link elements don't rely on this class and I'm sure we don't want to have a different styling for the restart link case. A quick search on MXR showed me that it's only this single element which makes use of the detail class: http://mxr.mozilla.org/mozilla-central/search?string=class=%22detail&find=xml It's only this extensions.xml file. Class details: http://mxr.mozilla.org/mozilla-central/source/toolkit/themes/gnomestripe/mozapps/extensions/extensions.css#345 What's the reason for having this extra class?
Assignee | ||
Comment 5•14 years ago
|
||
Ok, talked with Blair and we should keep it if other themes want to handle the button-link differently. Here an updated patch.
Attachment #478682 -
Attachment is obsolete: true
Attachment #479345 -
Flags: review?(dtownsend)
Comment 6•14 years ago
|
||
Comment on attachment 479345 [details] [diff] [review] Patch v2 Approved to land post b7
Attachment #479345 -
Flags: review?(dtownsend)
Attachment #479345 -
Flags: review+
Attachment #479345 -
Flags: approval2.0+
Assignee | ||
Updated•14 years ago
|
Whiteboard: [has patch][checkin-needed-post-b7]
Assignee | ||
Comment 7•14 years ago
|
||
As what we have seen by further working with Mozmill on the AOM, there are a couple more instances with an inconsistent usage of the btn suffix. It makes it really hard for us to get those elements. It would be really helpful when all the button would have that prefix, especially the ones with the same name in the different views (restart, undo, ...). As talked with Blair on IRC I have written a patch which I have now send to the tryserver. Once I get results which are hopefully green, I will upload a new patch tomorrow.
Whiteboard: [has patch][checkin-needed-post-b7] → [has patch]
Assignee | ||
Comment 8•14 years ago
|
||
As said in the comment before, the current naming scheme for buttons makes it hard for us to offer helper functions in the AddonsAPI for Mozmill tests, which makes it easier to create tests. With this patch all buttons get an "-btn" prefix, as long as they don't appear twice in different views. For those cases the class name should be sufficient. I have submitted the patch to the tryserver and all tests passed. I hope that we can get this patch integrated. If that's the case v2 of the patch is obsolete.
Attachment #481167 -
Flags: review?(dtownsend)
Assignee | ||
Updated•14 years ago
|
Summary: Restart and more button-link's are missing id or anonid → Update buttons in the Add-ons Manager to use a consistent id/anonid naming scheme
Assignee | ||
Updated•14 years ago
|
Whiteboard: [has patch] → [has patch][mozmill-needed]
Comment 9•14 years ago
|
||
Sorry, I don't understand why mozmill requires such consistency, can you explain?
Assignee | ||
Comment 10•14 years ago
|
||
It will make it much easier for us to retrieve elements of the same type by using a helper function like getAddonButton() or others, instead of having to customize it for each single element because of its different naming. If you do not want to have the latest patch attached in the code, we should at least get the first patch in. Having an id/anonid at all is much more important.
Comment 11•14 years ago
|
||
Comment on attachment 481167 [details] [diff] [review] Patch v3 We can probably take this so long as we do so a.s.a.p. but this patch does not apply to current trunk so it needs to be updated.
Attachment #481167 -
Flags: review?(dtownsend) → review-
Assignee | ||
Comment 12•14 years ago
|
||
The reason why it failed to apply was the addition of synthesizeMouseAtCenter for EventUtils. I have updated the patch and Blair mentioned via IRC that he will be so kind to run the tests locally to make sure that I haven't missed anything. Thanks!
Attachment #481167 -
Attachment is obsolete: true
Attachment #482425 -
Flags: review?(dtownsend)
Assignee | ||
Comment 13•14 years ago
|
||
Now the patch of the correct size. The browser_details patch was not complete.
Attachment #482425 -
Attachment is obsolete: true
Attachment #482426 -
Flags: review?(dtownsend)
Attachment #482425 -
Flags: review?(dtownsend)
Assignee | ||
Comment 14•14 years ago
|
||
Comment on attachment 482426 [details] [diff] [review] PAtch v4.1 Tests fail. I will run all the tests tomorrow before attaching a new version of the patch.
Attachment #482426 -
Attachment is obsolete: true
Attachment #482426 -
Flags: review?(dtownsend)
Assignee | ||
Comment 15•14 years ago
|
||
Updated patch which fix all the remaining failures of the browser chrome tests.
Attachment #482512 -
Flags: review?(dtownsend)
Updated•14 years ago
|
Attachment #482512 -
Flags: review?(dtownsend)
Attachment #482512 -
Flags: review+
Attachment #482512 -
Flags: approval2.0+
Assignee | ||
Updated•14 years ago
|
Keywords: checkin-needed
Assignee | ||
Comment 16•14 years ago
|
||
Comment on attachment 479345 [details] [diff] [review] Patch v2 Marking as obsolete in favor of the larger patch.
Attachment #479345 -
Attachment is obsolete: true
Comment 17•14 years ago
|
||
Landed: http://hg.mozilla.org/mozilla-central/rev/0b6940d11a43
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [has patch][mozmill-needed] → [mozmill-needed]
Target Milestone: --- → mozilla2.0b8
Updated•14 years ago
|
Target Milestone: mozilla2.0b8 → mozilla2.0b7
Assignee | ||
Comment 18•14 years ago
|
||
No regressions happened with this change. Marking as verified fixed.
Status: RESOLVED → VERIFIED
You need to log in
before you can comment on or make changes to this bug.
Description
•