Closed Bug 921877 Opened 6 years ago Closed 6 years ago

first time call to registerProtocolHandler is broken as of a4e9c9c9dbf9

Categories

(Firefox :: File Handling, defect)

x86_64
Windows 8
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 27
Tracking Status
firefox25 --- unaffected
firefox26 + verified
firefox27 + verified
b2g-v1.2 --- fixed

People

(Reporter: timeless, Assigned: dougt)

References

()

Details

(Keywords: regression, verifyme)

Attachments

(3 files)

I'm trying to use irccloud, when i disable chatzilla and load irccloud, i get a bar which has a message which sounds like a question.

1. There are no buttons
2. The flow control to actually do what it's talking about is unreachable because the button that should be used was removed in http://hg.mozilla.org/mozilla-central/rev/a4e9c9c9dbf9
bah.
Attached patch bug_921877Splinter Review
are there ui tests that I could write to prevent this?
Attachment #813436 - Flags: review?(jaws)
Comment on attachment 813436 [details] [diff] [review]
bug_921877

Review of attachment 813436 [details] [diff] [review]:
-----------------------------------------------------------------

Yes, you can write browser-chrome mochitests for this. You may be able to look at http://mxr.mozilla.org/mozilla-central/source/browser/base/content/test/general/browser_datareporting_notification.js for a reference although that test uses the global notification bar while this code doesn't.

The patch looks fine, but we should have a test that ensures this doesn't break again.
Attachment #813436 - Flags: review?(jaws)
Attached patch bug_921877_testsSplinter Review
Attachment #813994 - Flags: review?(jaws)
Comment on attachment 813436 [details] [diff] [review]
bug_921877

Review of attachment 813436 [details] [diff] [review]:
-----------------------------------------------------------------

"The patch looks fine" == r+
Attachment #813436 - Flags: review+
Comment on attachment 813994 [details] [diff] [review]
bug_921877_tests

Review of attachment 813994 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with the nits below addressed :)

::: browser/base/content/test/general/browser_registerProtocolHandler_notification.js
@@ +9,5 @@
> +    "browser/base/content/test/general/browser_registerProtocolHandler_notification.html";
> +
> +    // remember notification control is added in a setTimeout(0) call.  wait a
> +    // bit before testing (100ms)
> +    setTimeout(function() {

You should use the waitForCondition function here instead of setTimeout as it is a bit more robust with random timing changes.

@@ +14,5 @@
> +
> +        let notificationBox = window.gBrowser.getNotificationBox();
> +        let notification = notificationBox.getNotificationWithValue(notificationValue);
> +        ok(notification, "Notification box should be displayed");
> +        is(notification.type, "info", "We expect to this notification to have the type of 'info'.");

s/to //

@@ +15,5 @@
> +        let notificationBox = window.gBrowser.getNotificationBox();
> +        let notification = notificationBox.getNotificationWithValue(notificationValue);
> +        ok(notification, "Notification box should be displayed");
> +        is(notification.type, "info", "We expect to this notification to have the type of 'info'.");
> +        isnot(notification.image, null, "We expect to this notification to have an icon.");

s/to //

@@ +25,5 @@
> +        is(buttons.length, 1, "We expect to see one button.");
> +
> +        let button = buttons[0];
> +        isnot(button.label, null, "We expect to the add button to have a label.");
> +        todo_isnot(button.accesskey, null, "We expect to the add button to have a accesskey.");

lol, ok s/expect to /expect / on all of these comments :)
Attachment #813994 - Flags: review?(jaws) → review+
(In reply to Doug Turner (:dougt) from comment #6)
> "The patch looks fine" == r+

You know that's not always the case :) Lack of test coverage is a valid reason to withhold r+/prevent a patch from landing - in the case of a regression fix obviously the bar is a little different, and good judgement should prevail. I have no issue what with happened here, but I don't want your comment to be misinterpreted.
Yes, that is precisely why I waited to grant r+ until after I saw the tests. And sometimes while writing a test people realize that the other code needs to be changed.
> I have no issue what with happened here, but I don't want your comment to be misinterpreted.

good.
https://hg.mozilla.org/mozilla-central/rev/17ac833f18bf
https://hg.mozilla.org/mozilla-central/rev/010bebab0124
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 27
Keywords: verifyme
I see that this bug has a mochitest. Is there any need for manual testing here? 
I actually did test this manually and I found that when Add Application appears you have to click on that twice so it disappears completely. Is this normal?
Dropping verifyme for now.
Flags: needinfo?(doug.turner)
Keywords: verifyme
uplift nom please, with risk assessment.
lsblakk,
we must land this where ever I broke it.  basically, very simple patch with little risk at all.  if we don't take it, than it means that web sites can't define protocol handlers!
Flags: needinfo?(doug.turner) → needinfo?(lsblakk)
(In reply to Doug Turner (:dougt) from comment #16)
> lsblakk,
> we must land this where ever I broke it.  basically, very simple patch with
> little risk at all.  if we don't take it, than it means that web sites can't
> define protocol handlers!

That's fine - just nominate the patch(es) that are need for approval-mozilla-beta ? since that's the remaining branch marked affected.
Flags: needinfo?(lsblakk) → needinfo?(doug.turner)
Keywords: verifyme
Comment on attachment 813436 [details] [diff] [review]
bug_921877

[Triage Comment]
going to approve this for mozilla-beta uplift without an official nomination given Doug's comment provides risk assessment and we want this for the second-to-last beta on Monday.
Attachment #813436 - Flags: approval-mozilla-beta+
Flags: needinfo?(doug.turner)
Depends on: 942518
I verified this bug on

FF 26.08
Os Win 8 x64

I opened the irccloud page and the bar message has the Add Application button. Using the button adds the irccloud application for irc and ircs links.
I verified this bug on

Aurora
User Agent: Mozilla/5.0 (Windows NT 6.2; WOW64; rv:27.0) Gecko/20100101 Firefox/27.0
Build Id:20131128004001
Os Win 8 x64

I opened the irccloud page and the bar message has the Add Application button. Using the button adds the irccloud application for irc and ircs links.
You need to log in before you can comment on or make changes to this bug.