Closed
Bug 471404
Opened 16 years ago
Closed 16 years ago
"Remove All Reports" shouldn't be available if there aren't any reports
Categories
(Toolkit :: Crash Reporting, defect)
Toolkit
Crash Reporting
Tracking
()
RESOLVED
FIXED
mozilla1.9.2a1
People
(Reporter: dao, Assigned: dao)
References
Details
(Keywords: polish, Whiteboard: [polish-easy] [polish-interactive])
Attachments
(2 files)
4.06 KB,
patch
|
ted
:
review+
beltzner
:
approval1.9.1-
|
Details | Diff | Splinter Review |
3.07 KB,
patch
|
dao
:
review+
|
Details | Diff | Splinter Review |
"Remove All Reports" shouldn't be available if there aren't any reports. (It even produces an error in the noConfig case, because reportsDir is undefined.)
Attachment #354693 -
Flags: review?(ted.mielczarek)
Updated•16 years ago
|
Attachment #354693 -
Flags: review?(ted.mielczarek) → review+
Assignee | ||
Comment 2•16 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/77084056e89a
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.2a1
Comment 3•15 years ago
|
||
Comment on attachment 354693 [details] [diff] [review] patch We'd like to see this polish fix on branch, so request approval.
Attachment #354693 -
Flags: approval1.9.1?
Comment 4•15 years ago
|
||
Comment on attachment 354693 [details] [diff] [review] patch Obviously sdwilsh's account was hacked, as he asked for approval for this patch that doesn't have a test.
Attachment #354693 -
Flags: approval1.9.1? → approval1.9.1-
Updated•15 years ago
|
Flags: in-testsuite?
Comment 5•15 years ago
|
||
In hindsight, yeah, it should be testable. Probably a browser chrome test that: 1) cleans everything out of the Crash Reports/submitted dir 2) loads about:crashes, asserts that "remove all reports" button is hidden, closes about:crashes 3) adds an entry to Crash Reports/submitted 4) loads about:crashes, asserts that "remove all reports" button is visible, closes about:crashes
Comment 6•15 years ago
|
||
This test builds on some test utils added in a patch on bug 378528.
Attachment #382155 -
Flags: review?(dao)
Assignee | ||
Comment 7•15 years ago
|
||
Comment on attachment 382155 [details] [diff] [review] browser chrome test #clear-reports could have visibility:hidden or be completely absent from the document, and this test would fail. It's that kind of test that's more likely to cost rather than save development time in the future :( >+ if (visible) { >+ is(style.display, "block", "clear reports button is visible"); At least, this should be isnot(..., "none", ...), so that it won't fail if #clear-reports isn't floated.
Attachment #382155 -
Flags: review?(dao) → review+
Comment 8•15 years ago
|
||
Open to better suggestions, certainly. I don't actually write a lot of browser chrome tests...
Assignee | ||
Comment 9•15 years ago
|
||
Untested, but something like this should be more robust: function check_clear_visible(tab, aVisible) { let doc = gBrowser.getBrowserForTab(tab).contentDocument; let visible = false; let button = doc.getElementById("clear-reports"); if (button) { let style = doc.defaultView.getComputedStyle(button, ""); if (style.display != "none" && style.visibility == "visible") visible = true; } is(visible, aVisible, "clear reports button is " + aVisible ? "visible" : "hidden"); }
Comment 10•15 years ago
|
||
Thanks, I'll fix the test to use something like that before landing.
Updated•15 years ago
|
Flags: in-testsuite? → in-testsuite+
Comment 11•15 years ago
|
||
Pushed the test to m-c: http://hg.mozilla.org/mozilla-central/rev/f3bf98776109 Feel free to re-nom for 1.9.1.1 now that we have test coverage.
You need to log in
before you can comment on or make changes to this bug.
Description
•