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)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla1.9.2a1

People

(Reporter: dao, Assigned: dao)

References

Details

(Keywords: polish, Whiteboard: [polish-easy] [polish-interactive])

Attachments

(2 files)

Attached patch patchSplinter 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)
Keywords: polish
Whiteboard: [polish-easy] [polish-interactive]
Attachment #354693 - Flags: review?(ted.mielczarek) → review+
http://hg.mozilla.org/mozilla-central/rev/77084056e89a
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.2a1
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 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-
Flags: in-testsuite?
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
This test builds on some test utils added in a patch on bug 378528.
Attachment #382155 - Flags: review?(dao)
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+
Open to better suggestions, certainly. I don't actually write a lot of browser chrome tests...
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");
}
Thanks, I'll fix the test to use something like that before landing.
Flags: in-testsuite? → in-testsuite+
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.

Attachment

General

Created:
Updated:
Size: