921877 - first time call to registerProtocolHandler is broken as of a4e9c9c9dbf9

Status: RESOLVED FIXED

(Firefox :: File Handling, defect)

Description

[timeless]

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

Comment 1

[timeless]

Attachment: Picture of loading irccloud and seeing a bar about adding the irc protocol handler

Comment 2

[Doug Turner (:dougt)]

bah.

Comment 3

[Doug Turner (:dougt)]

Attachment: bug_921877

are there ui tests that I could write to prevent this?

Comment 4

[Jared Wein [:jaws] (please needinfo? me)]

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.

Comment 5

[Doug Turner (:dougt)]

Attachment: bug_921877_tests

Comment 6

[Doug Turner (:dougt)]

Comment on attachment 813436 [details] [diff] [review]
bug_921877

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

"The patch looks fine" == r+

Comment 7

[Jared Wein [:jaws] (please needinfo? me)]

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 :)

Comment 8

[Doug Turner (:dougt)]

works locally, lets try:
  https://tbpl.mozilla.org/?tree=Try&rev=306b85176c92

Comment 9

[:Gavin Sharp [email: [e-mail]]]

(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.

Comment 10

[Jared Wein [:jaws] (please needinfo? me)]

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.

Comment 11

[Doug Turner (:dougt)]

> I have no issue what with happened here, but I don't want your comment to be misinterpreted.

good.

Comment 12

[Doug Turner (:dougt)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/17ac833f18bf
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/010bebab0124

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/17ac833f18bf
https://hg.mozilla.org/mozilla-central/rev/010bebab0124

Comment 14

[Bogdan Maris, Desktop QA]

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.

Comment 15

[Lukas Blakk [:lsblakk] use ?needinfo]

uplift nom please, with risk assessment.

Comment 16

[Doug Turner (:dougt)]

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!

Comment 17

[Lukas Blakk [:lsblakk] use ?needinfo]

(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.

Comment 18

[Lukas Blakk [:lsblakk] use ?needinfo]

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.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/e71c3c1d1a75
https://hg.mozilla.org/releases/mozilla-beta/rev/e1cb9e41e2fd

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/e71c3c1d1a75
https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/e1cb9e41e2fd

Comment 21

[Catalin Varga [QA][:VarCat]]

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.

Comment 22

[Catalin Varga [QA][:VarCat]]

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.