1270416 - RegisterProtocolHandler Flow Enhancement

Status: REOPENED

(Firefox :: File Handling, defect, P3)

Description

[Eden Chuang[:edenchuang]]

When a protocol handler calls RegisterProtocolHandler and user confirms the registration, set the protocol handler as the default handler.

Comment 1

[Eden Chuang[:edenchuang]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d3357d6d5e40

Comment 2

[Eden Chuang[:edenchuang]]

Attachment: Bug1270416-Set_The_Registered_Protocol_Handler_As_The_Default_Handler.

WIP patch, and its tryserver result

https://treeherder.mozilla.org/#/jobs?repo=try&revision=744826af72e6&selectedJob=21018249

Maybe some test golden need to be updated.

Comment 3

[Eden Chuang[:edenchuang]]

According to UX spec change, the feature is no more needed. Close this bug.

Comment 4

[Eden Chuang[:edenchuang]]

Reopen the bug according to the spec

https://mozilla.invisionapp.com/share/4Y6ZZH1E8#/screens/162255220

Comment 5

[Eden Chuang[:edenchuang]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=907f117db6c4

Comment 6

[Eden Chuang[:edenchuang]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8b74ce07863e

Comment 7

[Eden Chuang[:edenchuang]]

Attachment: Bug1270416-Set_The_Registered_Protocol_Handler_As_The_Default_Handler.

The patch changes the behavior after clicking the "OK" button on the notification bar. Once the "OK" is pressed, the website/webapp will be set as the default handler for the specified web protocol.

And the following is the try result
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8adccfd303f6

Comment 8

[Eden Chuang[:edenchuang]]

Bryant, this bug is waiting for spec lock down.

https://mozilla.invisionapp.com/share/4Y6ZZH1E8#/screens/162255220

Once the spec is finalized, please clean the NI flag to let me know the patch can be pushed into review process. Thanks.

Comment 9

[William Hsu [:whsu]]

This bug is shown in Triage dashboard.
I believe Bryant is working on it. Keep monitoring/NI.

Comment 10

[Eden Chuang[:edenchuang]]

Change the bug title to the proper one.

Comment 11

[Eden Chuang[:edenchuang]]

Attachment: Original_Notification_Bar

Comment 12

[Eden Chuang[:edenchuang]]

Attachment: New_Notification_Bar

Comment 13

[Eden Chuang[:edenchuang]]

Attachment: Timeout_Notification

Comment 14

[Eden Chuang[:edenchuang]]

Hi Gijs and Paolo

After met you at London, UX team proposed some enhancements for protocol/content handler registration flow.
Following I will describe the original design at first, and then introduce these enhancements. Could you give some comments or suggestions? Thank you.

When RegisterProtocolHandler is called, a notification bar popup at the top of web page to ask if user wants to add the web app into the possible handler list or not. See the attached "Original_Notification_Bar".

User can press "Add application" button or "X" button.
If "Add application" is pressed, the web app is added into the possible handler list and the protocol default behavior is set as "Always ask". Because the default behavior is set as "Always ask", the new handler has a chance to be the default handler at the next time the same protocol type link is opened.
If "X" is pressed, close the notification bar and make no difference.

Some UX enhancements are considered.
1. If user wants the handler to be the default handler, don't let user set it at next time.

2. If user wants manage the possible handlers, give user a direction to open the "Application Panel".

3. Don't bother user if they really don't want to register the web app.
   "X" just close the notification bar. When user visits the same web app, the notification bar popup again.

To support these enhancements, the new notification bar design is proposed, See the attached "New_Notification_Bar". It contains three buttons, "Never ask again", "Set as default" and "X".

If "Never ask again" is pressed, nsIHandlerInfo probably needs a new attribute, maybe "BlackList", and related APIs, for example getBlackList, to make sure the notification bar doesn't popup again when visiting web sites in BlackList.

If "Set as default" is pressed, directly set the handler as the default handler, and popup a timeout notification to info user the handler will handle all the protocol type links,see the attached "Timeout_Notification". The timeout notification has a link "Learn more" to "Application panel", such that user can manage handlers, if they just want register but not set as default.

If "X" is pressed, keep the original behavior, just close the notification bar.

For RegisterContentHandler calls, we have the similar enhancements. 

If these enhancements are agreed, they need to modify the RegisterProtocolHandler and RegisterContentHandler, and I guess the Gijs will be the reviewer, or Gijs can suggest a reviewer to us. Thanks.

Comment 15

[:Gijs (he/him)]

(In reply to Eden Chuang[:edenchuang] from comment #14)
> Hi Gijs and Paolo
> 
> After met you at London, UX team proposed some enhancements for
> protocol/content handler registration flow.
> Following I will describe the original design at first, and then introduce
> these enhancements. Could you give some comments or suggestions? Thank you.
> 
> When RegisterProtocolHandler is called, a notification bar popup at the top
> of web page to ask if user wants to add the web app into the possible
> handler list or not. See the attached "Original_Notification_Bar".
> 
> User can press "Add application" button or "X" button.
> If "Add application" is pressed, the web app is added into the possible
> handler list and the protocol default behavior is set as "Always ask".
> Because the default behavior is set as "Always ask", the new handler has a
> chance to be the default handler at the next time the same protocol type
> link is opened.
> If "X" is pressed, close the notification bar and make no difference.
> 
> Some UX enhancements are considered.
> 1. If user wants the handler to be the default handler, don't let user set
> it at next time.
> 
> 2. If user wants manage the possible handlers, give user a direction to open
> the "Application Panel".
> 
> 3. Don't bother user if they really don't want to register the web app.
>    "X" just close the notification bar. When user visits the same web app,
> the notification bar popup again.
> 
> To support these enhancements, the new notification bar design is proposed,
> See the attached "New_Notification_Bar". It contains three buttons, "Never
> ask again", "Set as default" and "X".
> 
> If "Never ask again" is pressed, nsIHandlerInfo probably needs a new
> attribute, maybe "BlackList", and related APIs, for example getBlackList, to
> make sure the notification bar doesn't popup again when visiting web sites
> in BlackList.

The nsIHandlerInfo API and its friends (and ugh mimetypes.rdf) are already trying to do everything and the kitchen sink and have issues (cf. bug 332690). I would like to avoid adding more complexity to them if not absolutely necessary. AIUI we can use the Permissions API for this, which would avoid having to reinvent the wheel. Alternatively, it seems the code already has a pref-based list of protocols for which handlers cannot be added. We could expand that system if for some reason the Permissions API is not suitable.

> If "Set as default" is pressed, directly set the handler as the default
> handler, and popup a timeout notification to info user the handler will
> handle all the protocol type links,see the attached "Timeout_Notification".
> The timeout notification has a link "Learn more" to "Application panel",
> such that user can manage handlers, if they just want register but not set
> as default.

I'm not sure the extra popup notification is warranted. We don't typically prompt for "oh hey, we did this thing and it worked". I think just making the new handler the default when the user clicks that button is fine. Notifications that disappear after a certain amount of time also have UX issues (e.g. bug 1290011), which are even worse if there is no obvious way for the user to make the notification appear again (which there won't be in this case, because now the handler is added and so it cannot be added again to produce the same notification). So on the whole, I would prefer that the we don't show a second notification.

> If "X" is pressed, keep the original behavior, just close the notification
> bar.
> 
> For RegisterContentHandler calls, we have the similar enhancements. 
> 
> If these enhancements are agreed, they need to modify the
> RegisterProtocolHandler and RegisterContentHandler, and I guess the Gijs
> will be the reviewer, or Gijs can suggest a reviewer to us. Thanks.

Sure, I can probably review changes to those files.

Comment 16

[:Paolo Amadini]

(In reply to :Gijs Kruitbosch (PTO recovery mode) from comment #15)
> AIUI we can use the Permissions API for this

Using the Permissions API looks good to me. I also wonder if we can use the unified Permissions Notifications and Permission UI in the Control Center that the Privacy and Security team is working on.

One of the features of the new permissions design is that if the user chooses "Never Ask Again", they can revert this decision from the Control Center. In the notification bar design, there doesn't seem to be a way for the user to restore the permission.

> I'm not sure the extra popup notification is warranted. We don't typically
> prompt for "oh hey, we did this thing and it worked". I think just making
> the new handler the default when the user clicks that button is fine.

I agree, and I think the "Learn more" link should be offered before the user has to make the choice.

One possible issue with handler registration is that any website on the Internet can ask the user to redirect certain types of links to it. For example, any website could register for the "tel:" protocol and see every phone number the user clicks on, even for example if it's a private contact.

With the new design that sets the handler as the default, this is more likely to happen by accident even if you have another website already working and registered for this type of link.

Also, differently from the handler choice screen that appears when you actually try to call someone from your phonebook application, the notification bar can appear out of context on any arbitrary website. I don't think users would realize what a "tel" link is if the notification bar in attachment 8779984 [details] is shown on a malicious website that shows a conflicting message in the content area.

In addition to being a security and privacy issue, this might break user experience arbitrarily. This is why we should review this design carefully, especially if we want to use the Permissions Notifications UI. Other permissions, when granted, only affect the website you are currently visiting. Dangerous permissions like "Install Add-ons" are instead set to "block" by default.

Comment 17

[:Paolo Amadini]

Maybe a more complete design to improve user experience should include a description provided by Firefox for common protocol types ("mailto:", "tel:", ...) that are commonly used for interaction between web applications.

Or we could require a WebExtension to be installed for handling protocols, this would provide users with the additional safety of our add-on review process. Maybe this option was also discussed before.

How common is registerProtocolHandler on major websites these days? How is this API handled by other browsers?

Comment 18

[Panos Astithas (he/him) [:past] (please ni?)]

(In reply to :Paolo Amadini from comment #17)
> Or we could require a WebExtension to be installed for handling protocols,
> this would provide users with the additional safety of our add-on review
> process. Maybe this option was also discussed before.

Be careful with that path. You may recall the discussions around the screen sharing whitelist case, where the consensus was that requiring an add-on usually has worse security trade offs.

Comment 19

[Eden Chuang[:edenchuang]]

(In reply to :Paolo Amadini from comment #16)
> (In reply to :Gijs Kruitbosch (PTO recovery mode) from comment #15)
> > AIUI we can use the Permissions API for this
> 
> Using the Permissions API looks good to me. I also wonder if we can use the
> unified Permissions Notifications and Permission UI in the Control Center
> that the Privacy and Security team is working on.

The method I mentioned in comment 14 is just a possible implementation.
Permission API seems a good solution, but I am not familiar with it, so I will study it first, then I will come out an implementation and discuss with you. 

> One of the features of the new permissions design is that if the user
> chooses "Never Ask Again", they can revert this decision from the Control
> Center. In the notification bar design, there doesn't seem to be a way for
> the user to restore the permission.

Yes, the new notification design lacks the restore mechanism. But UX team is also designing the new "Application Panel", I think the restore mechanism should be considered in the new design. I will check it with UX team again. Here UX team wants to keep this notification bar being simple. 

> > I'm not sure the extra popup notification is warranted. We don't typically
> > prompt for "oh hey, we did this thing and it worked". I think just making
> > the new handler the default when the user clicks that button is fine.

I guess the propose of the popup notification is having a way to indicate user to "Application Panel" if user wants to do some advanced settings. But I agree it seems not 

> I agree, and I think the "Learn more" link should be offered before the user
> has to make the choice.
> 
> One possible issue with handler registration is that any website on the
> Internet can ask the user to redirect certain types of links to it. For
> example, any website could register for the "tel:" protocol and see every
> phone number the user clicks on, even for example if it's a private contact.
> 
> With the new design that sets the handler as the default, this is more
> likely to happen by accident even if you have another website already
> working and registered for this type of link.

I think the original design has the same problem, because the default behavior is set as "Always Ask". If I had already set a default handler for the protocol, I think it very strange that popup the "Launch Application" dialog when next time I open the protocol link.

Why original design set the default behavior as "Always ask" when the RegisterProtocolHandler is agreed by user? I guess the logic is if user register the handler, user probably wants to use this handler. However, if there is a default handler, user has no chance to use the new one, except user changes the default handler or behavior in "Application Panel", so the original design set the default behavior to "Always Ask" to let user use the new handler by "Launch Application" dialog. 

So, I think UX team proposes the new design and wants to resolve the UX problem and make the process be more smooth.

> Also, differently from the handler choice screen that appears when you
> actually try to call someone from your phonebook application, the
> notification bar can appear out of context on any arbitrary website. I don't
> think users would realize what a "tel" link is if the notification bar in
> attachment 8779984 [details] is shown on a malicious website that shows a
> conflicting message in the content area.

I think the problem is not "Set as default" action, the problem is no clear information for user to judge if they should press "Set as default" or not. If we can tell user clearly what will happen and risk if they press "Set as default", the problem you mentioned might be resolved. For the case, when the website wants to register "tel:", we need to tell user "Every phone number the you clicks will be seen by the website", such that user can decide to press "Set as default", "X", or "Never Ask Again". Paolo, do you think is it better?

> In addition to being a security and privacy issue, this might break user
> experience arbitrarily. This is why we should review this design carefully,
> especially if we want to use the Permissions Notifications UI. Other
> permissions, when granted, only affect the website you are currently
> visiting. Dangerous permissions like "Install Add-ons" are instead set to
> "block" by default.

I agree, that's why I need your help. :)

> Maybe a more complete design to improve user experience should include a description provided by Firefox > for common protocol types ("mailto:", "tel:", ...) that are commonly used for interaction between web
> applications.

Yes, I agree that clear and user friendly description for these protocols are needed. I will discuss with UX team how to improve it.

> Or we could require a WebExtension to be installed for handling protocols, this would provide users with
> the additional safety of our add-on review process. Maybe this option was also discussed before.

(In reply to : Panos Astithas from comment #18)
> Be careful with that path. You may recall the discussions around the screen sharing whitelist case, where > the consensus was that requiring an add-on usually has worse security trade offs.

I am not sure if using WebExtension is a good way or not, but I want to make things be simple. According to Panos's comment, I might not try this way first, but we can keep in mind.

>How common is registerProtocolHandler on major websites these days? How is this API handled by other browsers?

I am not sure how common registerProtocolHandler is. Might be we can do some study, but I don't think this should affect the improvement. registerProtocolHandler is handled by Chrome and Opera like the attached "RegisterProtocolHandler_By_Others". In fact, I think the design is much likely our new design.

Thanks Paolo and Gijs, you gave us very helpful suggestions.

Following I will study how to implement blacklist feature with Permission API, and discuss with UX how to make the description be more clear to help user judge if he can press "Set As Default" button.

Comment 20

[Eden Chuang[:edenchuang]]

Attachment: RegisterProtocolHandler_By_Others

Comment 21

[:Gijs (he/him)]

I don't really have answers about the intent/reasoning behind the current/original design and 'set as default'.

It does seem that Chrome treats this more as essentially a permissions request from the website, which suggests that we could reuse that infrastructure on our side (ie make this part of control center with a popup notification instead of the notification bar), which might simplify both the implementation and the ability for users to revoke the permission and/or set a handler as the default. Generally, I think we tend to use popup notifications more often than notification bars these days, though I'm not an expert on why we pick one or the other...

Comment 22

[:Paolo Amadini]

(In reply to Eden Chuang[:edenchuang] from comment #20)
> Created attachment 8781023 [details]

One of the features of this design used by Chrome is that it addresses the security issues I raised in comment 16, because it makes it impossible to set the handler as default with just one click. You need to confirm your choice by changing the radio button first.

I think our new permissions notification model will fit well with what we need here, if we add this additional safeguard. We'll discuss this approach with the front-end security designers during one of our meetings.

(In reply to Eden Chuang[:edenchuang] from comment #19)
> Why original design set the default behavior as "Always ask" when the
> RegisterProtocolHandler is agreed by user?

One of the reasons is to show the choice in context, the other to make it impossible to change the preferences accidentally with one click. We cannot preserve the former with a permission notification, but there we have the space required to add clear explanatory text like the one you suggested. We can also add a "Learn more" link easily.

> > How common is registerProtocolHandler on major websites these days?
> 
> I am not sure how common registerProtocolHandler is. Might be we can do some
> study, but I don't think this should affect the improvement.

To clarify, I wasn't referring to absolute usage, but how common it is for major websites to use registerProtocolHandler versus asking to install an add-on, a Desktop application, or an app to solve the problem of handling certain link types, like e-mail. I agree we don't need to know this because, judging from how Chrome handles this, we're probably on the right track for an easy solution anyways.

Comment 23

[:Paolo Amadini]

Chrome also gives us a suggestion for the anchor icon to use, these overlapping diagonal squares :-)

Comment 24

[Eden Chuang[:edenchuang]]

I did some study on Permissions API. I think it a good solution for registerprotocolhandler/registercontenthandler implementation. But, Permissions API currently is not fully implemented. 

https://developer.mozilla.org/en-US/docs/Web/API/Permissions

The most important method request() currently is not supported, such that we currently can not implement with Permissions API. I am not sure when Permissions API will be ready, so I suggest to open a following bug for implementing with Permissions API. For now, I might provide a workaround to fulfill the UX requirements.

I also reflected following UI issues to UX team.

1. "Never Ask Again" restore issue
2. Popup notification issue
3. More clear description and move "learn more" before press buttons.

Bryant, please give some comments on these UI issues. Thanks.

Comment 25

[:Gijs (he/him)]

(In reply to Eden Chuang[:edenchuang] from comment #24)
> I did some study on Permissions API. I think it a good solution for
> registerprotocolhandler/registercontenthandler implementation. But,
> Permissions API currently is not fully implemented. 
> 
> https://developer.mozilla.org/en-US/docs/Web/API/Permissions

This is really not what I meant. I meant that we should use the same UI as we use for other permissions websites request (device access like camera/microphone, geolocation, ...) and the same internal thing to store those permissions in (nsIPermissionManager).

Comment 26

[Eden Chuang[:edenchuang]]

Sorry, I misunderstanding the term of "Permissions API".

After taking a look on nsIPermissionManager, I think it's really easy to achieve what we want by using nsIPermissionManager. And it also provides the restore mechanism to user.

Here are some things that I am thinking about.

1. Could it be possible that we still need to save denied handler information in mimeTypes.rdf(or mimeTypes.json in the future)? For example, if UX finally decide to show the denied handlers on Application panel, and these information are not saved by nsIPermissionManager, i.e. handler description. If the answer is "Yes", it can not avoid adding some new attribute in nsIHandlerInfo. 

2. Could we remove information saved in mimeTypes.rdf by using nsIPermissionManager?

Comment 27

[:Gijs (he/him)]

(In reply to Eden Chuang[:edenchuang] from comment #26)
> Sorry, I misunderstanding the term of "Permissions API".
> 
> After taking a look on nsIPermissionManager, I think it's really easy to
> achieve what we want by using nsIPermissionManager. And it also provides the
> restore mechanism to user.
> 
> Here are some things that I am thinking about.
> 
> 1. Could it be possible that we still need to save denied handler
> information in mimeTypes.rdf(or mimeTypes.json in the future)? For example,
> if UX finally decide to show the denied handlers on Application panel, and
> these information are not saved by nsIPermissionManager, i.e. handler
> description.

Having web-provided handler description here is likely not a good idea anyway. We've moved away from having websites explain their requests for access to sensitive data or APIs because it's a recipe for phishing and other potential security problems (e.g. we no longer show the website-provided 'reason' for preventing you from leaving a page, and we do not show a website-provided string (beyond the domain name, of course) when requesting any of the device, geolocation or other permissions that I mentioned earlier).

Even if we think it provides context when websites ask for the registration, if the user has already denied it, I don't think there's much point showing the website description (rather than Firefox-provided strings for things like 'tel:' or other common protocols).

> 2. Could we remove information saved in mimeTypes.rdf by using
> nsIPermissionManager?

I don't know the answer to this, mostly because I don't really know what we're currently storing in mimeTypes.rdf for protocols.

Comment 29

[Eden Chuang[:edenchuang]]

UX team are designing the new user interface with control center.
The draft spec was discussed with control center UX, Aislinn. And it needs some adjustments to align with control center design concept.

Comment 30

[Eden Chuang[:edenchuang]]

Attachment: Handler Registration and Management

The attached "Handler Registration and Management" is the new UI design with control center which suggested by Gijs and Paolo. And Bryant had already discussed this spec with control center UX Aislinn.

Gijs and Paolo, would you please take a look on it and give any comments about this new UI design if you have any concerns. If there is no critical issues, I will start to implement it.

Comment 31

[:Gijs (he/him)]

I have some questions:

- the design says "Whenever users set another website as the default application... the settings in control center will be updated accordingly" - but there doesn't seem to be a design for this case, as the app is clearly neither blocked nor allowed to handle *All* email links. There also doesn't seem to be a design for the control center in the case where the user clicks "manage application" (which implicitly accepts adding the app as a possible handler without making it the default handler), where you end up in a similar situation.

- aren't we violating the principle of "least surprise" when clicking "Manage Application" silently adds the application in question as an option for that protocol?

- which protocols are we supporting? The text says "email" everywhere, not "mailto", and so there must be some kind of mapping.

- if we have a mapping, maybe we should use clearer icons as well? The little square with an arrow isn't very "mail"-y to me. Maybe some version of our existing email link icon or some other use of the 'envelope' idiom would be more appropriate?

- the design says that with "Never allow", "the icon remains in place with a [slash] through it. Is this really necessary? The user can alter their decision from the control center - I'm not sure we should be taking up space in the location bar permanently for such a site.

Comment 32

[:Paolo Amadini]

(In reply to :Gijs Kruitbosch from comment #31)
> - the design says that with "Never allow", "the icon remains in place with a
> [slash] through it. Is this really necessary? The user can alter their
> decision from the control center - I'm not sure we should be taking up space
> in the location bar permanently for such a site.

I can answer this question - this is part of the new design of permissions in the Control Center and we do it for all permissions. It's intentional that we take up the horizontal space, so that we provide a breadcrumb for the user to find out how to revert their decision. Also, it's strictly an improvement over the current situation of taking up vertical real estate with the repeated notification bar.

Comment 33

[:Paolo Amadini]

I agree with the concerns about the "Manage Application" link, and I think this can be left out from the initial version, because there isn't such a link in the notification bar. This work can be split to its own bug and implemented after the rest of the permission-based interface is ready.

One major concern is that, due to the particular security characteristics of this permission, that allows one site to access links located on other sites, we should use our checkbox capability and make accepting a two-click process. For reference, Chrome also uses a two-click process.

Comment 34

[:Paolo Amadini]

(In reply to :Gijs Kruitbosch from comment #31)
> - which protocols are we supporting? The text says "email" everywhere, not
> "mailto", and so there must be some kind of mapping.
> 
> - if we have a mapping, maybe we should use clearer icons as well? The
> little square with an arrow isn't very "mail"-y to me. Maybe some version of
> our existing email link icon or some other use of the 'envelope' idiom would
> be more appropriate?

I think the actual permission should be a single one for all schemes, and just be used to block the website from asking again. This has the advantage of reducing spamming potential when the site calls the API many times and changes the scheme.

So, the permission line in the Control Center would not mention which specific scheme is being blocked.

Given the above, for the icon I think we can use a generic abstract icon just like Chrome does. My guess is that Chrome's icon tries to be a link or something. The one I see in the specification is good for me.

The multiple prompts case I see in the specification is not yet implemented for permissions, so it might have to wait until the Security and Privacy team figures out how to handle it. For now, if the site asks for multiple schemes, blocking the first would automatically prevent the other requests from showing. I think this is acceptable, unless we have a real use case in the wild where denying the first request and accepting the second is really needed, and is impractical to be fixed by the website.

Comment 35

[:Gijs (he/him)]

(In reply to :Paolo Amadini from comment #34)
> (In reply to :Gijs Kruitbosch from comment #31)
> > - which protocols are we supporting? The text says "email" everywhere, not
> > "mailto", and so there must be some kind of mapping.
> > 
> > - if we have a mapping, maybe we should use clearer icons as well? The
> > little square with an arrow isn't very "mail"-y to me. Maybe some version of
> > our existing email link icon or some other use of the 'envelope' idiom would
> > be more appropriate?
> 
> I think the actual permission should be a single one for all schemes, and
> just be used to block the website from asking again. This has the advantage
> of reducing spamming potential when the site calls the API many times and
> changes the scheme.

I'm not sure I understand how this would work. Once the user allows this for one scheme, would we just make the website the default for all schemes? That doesn't sound attractive...

Comment 36

[:Paolo Amadini]

(In reply to :Gijs Kruitbosch from comment #35)
> I'm not sure I understand how this would work. Once the user allows this for
> one scheme, would we just make the website the default for all schemes? That
> doesn't sound attractive...

No, they would be asked for the next scheme with another prompt. We just cannot show how many other schemes the site will ask for (the "1 of 4" indication in the document).

We wouldn't store anything in the permission database for successful associations, in other words we don't have an "allowed" state for the permission. Whether we want to surface the current associations in the Control Center and allow them to be cancelled is another story, and probably again it can be moved to a follow-up. We don't have this functionality at present anyways.

Comment 37

[Eden Chuang[:edenchuang]]

(In reply to :Gijs Kruitbosch from comment #31)
> I have some questions:
> 
> - the design says "Whenever users set another website as the default
> application... the settings in control center will be updated accordingly" -
> but there doesn't seem to be a design for this case, as the app is clearly
> neither blocked nor allowed to handle *All* email links. 

I guess design concept is there is only one handler could be allowed at one time, it also means the allowed one is the default handler. For the others, they all should be blocked.

For the case that user allows a new website, the other websites' status should be updated correspondingly.
If the original status is "Allow", it should be updated as "Prompt" or "Deny". For other status, keep it as original. Permission update on other websites is a very special usage of control center, I never seen that before. I guess UX team want to let control center be another way to manage application association.  

Morpheus, could you give comments and explain the permission update design concept?

My concern about permission update is that it could surprise users if we don't notify this change to users?
I think we can do some message refinement for different situations. For the case that existing a default handler "mail.yahoo.com", we probably can show the message "Would you like to change "mail.yahoo.com" to "mail.google.com" to be the default application to open all email links?" instead of the original one.

> There also doesn't
> seem to be a design for the control center in the case where the user clicks
> "manage application" (which implicitly accepts adding the app as a possible
> handler without making it the default handler), where you end up in a
> similar situation.
> 
> - aren't we violating the principle of "least surprise" when clicking
> "Manage Application" silently adds the application in question as an option
> for that protocol?
> 

After check with Morpheus, the handler will not be added into possible handlers after clicking "Manage Application". Nothing will be changed on the original page, user still needs to select "Always Allow" or "Never Allow" on the original page. 
I agree Paolo's suggestion to left out this part from initial version, because I think the spec is still not well-defined. We can open a follow bug to trace the issue how to indicate user to "Application Panel" to get more information.

> - which protocols are we supporting? The text says "email" everywhere, not
> "mailto", and so there must be some kind of mapping.
> 
> - if we have a mapping, maybe we should use clearer icons as well? The
> little square with an arrow isn't very "mail"-y to me. Maybe some version of
> our existing email link icon or some other use of the 'envelope' idiom would
> be more appropriate?
> 

Yes, there is a string mapping for some special protocols, such as mailto and webcal, and a general message for other protocols. The message "Would you like ... to open all the email links?" is copied form current release version notification bar. Of course, we can do some message refinement to make it be more clear. The clear icon is a good suggestion, I will reflect it to UX.
I think UX team can refine the message and icon at the same time with implementation, so this shouldn't block the code implementation, right?

> - the design says that with "Never allow", "the icon remains in place with a
> [slash] through it. Is this really necessary? The user can alter their
> decision from the control center - I'm not sure we should be taking up space
> in the location bar permanently for such a site.

Thanks Paolo to answer this question. :)

(In reply to :Paolo Amadini from comment #34)

> I think the actual permission should be a single one for all schemes, and just be used to block the
> website from asking again. This has the advantage of reducing spamming potential when the site calls the 
> API many times and changes the scheme.
> 
> So, the permission line in the Control Center would not mention which specific scheme is being blocked.
>
> The multiple prompts case I see in the specification is not yet implemented for permissions, so it might 
> have to wait until the Security and Privacy team figures out how to handle it. For now, if the site asks 
> for multiple schemes, blocking the first would automatically prevent the other requests from showing. I 
> think this is acceptable, unless we have a real use case in the wild where denying the first request and 
> accepting the second is really needed, and is impractical to be fixed by the website.

Let me use a real case to figure out this design. For example, when you open irccloud, it asks two schemes, "irc" and "ircs". Then the first prompt popup for "irc" scheme, "Would you like ... to handle all "irc" links" message is shown to user. If user selects "Always Allow", the irccloud is set as the default handler for irc links. At this moment, if user can enter into Control Center, instead of "Open All irc links", a possible string, such as "Handler Registration", is shown in Control Center and it status is "Allowed". Then second prompt is popup for "ircs" scheme. If user selects "Never Allow", irccloud will not be the default handler, and the "Handler Registration" permission in Control Center becomes "Blocked". If irccloud ask the third scheme, i.e. "mailto", the prompt will not be shown, because the permission is "Blocked". In the end, irccloud is the default handler for irc links and original handler for ircs links, and "Handler Registration" permission for irccloud is "blocked". Does this example show the design correctly?

(In reply to :Paolo Amadini from comment #35)

> Whether we want to surface the current associations in the Control Center and allow them to be cancelled 
> is another story

I guess this is what UX team want to do in this spec.

Comment 38

[:Gijs (he/him)]

(In reply to Eden Chuang[:edenchuang] from comment #37)
> (In reply to :Gijs Kruitbosch from comment #31)
> > I have some questions:
> > 
> > - the design says "Whenever users set another website as the default
> > application... the settings in control center will be updated accordingly" -
> > but there doesn't seem to be a design for this case, as the app is clearly
> > neither blocked nor allowed to handle *All* email links. 
> 
> I guess design concept is there is only one handler could be allowed at one
> time, it also means the allowed one is the default handler. For the others,
> they all should be blocked.
> 
> For the case that user allows a new website, the other websites' status
> should be updated correspondingly.
> If the original status is "Allow", it should be updated as "Prompt" or
> "Deny". For other status, keep it as original. Permission update on other
> websites is a very special usage of control center, I never seen that
> before. I guess UX team want to let control center be another way to manage
> application association. 

I still don't follow. If you click "Manage application", if I understand the spec we add the website's protocol handler as an option for the user to choose, but it is not made the default.
Likewise, my expectation was that if you have previously made one website's protocol handler the default, and now you go to a different one and make that the default for the same profile, the first website would remain in the list in about:preferences (but is no longer the default).

Are you saying that "in the about:preferences list but not the default" should correspond to not allowing (always block) the use of registerProtocolHandler by that website? Because that seems wrong - there would be no feedback if the user clicked a button on the website to associate a protocol with the same website, because doing so would be blocked. Firefox would look broken.

I guess the correct state for the control center would be to just not list a permission, and prompt again on that site if registerProtocolHandler is called again, or something?

> Morpheus, could you give comments and explain the permission update design
> concept?
> 
> My concern about permission update is that it could surprise users if we
> don't notify this change to users?
> I think we can do some message refinement for different situations. For the
> case that existing a default handler "mail.yahoo.com", we probably can show
> the message "Would you like to change "mail.yahoo.com" to "mail.google.com"
> to be the default application to open all email links?" instead of the
> original one.

This is a good idea, I think.

> > There also doesn't
> > seem to be a design for the control center in the case where the user clicks
> > "manage application" (which implicitly accepts adding the app as a possible
> > handler without making it the default handler), where you end up in a
> > similar situation.
> > 
> > - aren't we violating the principle of "least surprise" when clicking
> > "Manage Application" silently adds the application in question as an option
> > for that protocol?
> > 
> 
> After check with Morpheus, the handler will not be added into possible
> handlers after clicking "Manage Application". Nothing will be changed on the
> original page, user still needs to select "Always Allow" or "Never Allow" on
> the original page. 

I don't understand what would be the point of the link to "Manage Application" in that case. I also believe this doesn't match the current spec, which says "link the page to protocol handler in Settings".

> I agree Paolo's suggestion to left out this part from initial version,
> because I think the spec is still not well-defined. We can open a follow bug
> to trace the issue how to indicate user to "Application Panel" to get more
> information.

If we are comfortable with making it hard/impossible for the user to add the handler but not make it the default, this is an option.

> > - which protocols are we supporting? The text says "email" everywhere, not
> > "mailto", and so there must be some kind of mapping.
> > 
> > - if we have a mapping, maybe we should use clearer icons as well? The
> > little square with an arrow isn't very "mail"-y to me. Maybe some version of
> > our existing email link icon or some other use of the 'envelope' idiom would
> > be more appropriate?
> > 
> 
> Yes, there is a string mapping for some special protocols, such as mailto
> and webcal, and a general message for other protocols. The message "Would
> you like ... to open all the email links?" is copied form current release
> version notification bar. Of course, we can do some message refinement to
> make it be more clear. The clear icon is a good suggestion, I will reflect
> it to UX.
> I think UX team can refine the message and icon at the same time with
> implementation, so this shouldn't block the code implementation, right?

Sure.

> (In reply to :Paolo Amadini from comment #34)
> 
> > I think the actual permission should be a single one for all schemes, and just be used to block the
> > website from asking again.
> Let me use a real case to figure out this design.

This is tricky to get right... :-(

Comment 39

[:Paolo Amadini]

(In reply to Eden Chuang[:edenchuang] from comment #37)
> Let me use a real case to figure out this design. [...]
> Does this example show the design correctly?

It does, with one correction: "Handler Registration: Allowed" would not be shown in the Control Center because it is the default value for the permission.

In practice, you have described an edge case, because you have chosen an artificial example that does not happen in practice. In other words in your example IRCCloud wants to register for the "mailto" protocol, which is not the case in reality, and the user wants to register only one of the protocols, which isn't particularly useful. I don't think we'll have websites that want to register for two unrelated protocols, so even if this interaction looks contrived it will rarely happen, and we definitely shouldn't optimize for it, though it's good to discuss it.

In the example of two protocols, if you want to register the site then you get asked two times in a row, and you have to accept both. Ideally we would like to ask only one time for both "irc" and "ircs" if the site does the request at the same time, but at present the Privacy and Security team hasn't implemented this multiple requests feature.

If you do not want to use the site, you deny the first request, and don't see the other one. These are the two common use cases.

Comment 40

[Morpheus Chen [:morpheus] UX]

Sorry for the late reply. Please find my comments inline.

(In reply to Eden Chuang[:edenchuang] from comment #37)
> (In reply to :Gijs Kruitbosch from comment #31)
> I guess design concept is there is only one handler could be allowed at one
> time, it also means the allowed one is the default handler. For the others,
> they all should be blocked.
> For the case that user allows a new website, the other websites' status
> should be updated correspondingly.
> If the original status is "Allow", it should be updated as "Prompt" or
> "Deny". For other status, keep it as original. Permission update on other
> websites is a very special usage of control center, I never seen that
> before. I guess UX team want to let control center be another way to manage
> application association.  
> 
> Morpheus, could you give comments and explain the permission update design
> concept?

Yes, the mechanism of redesigned UX works exactly like what you described above. Only one thing is different than what we expected: 

> If the original status is "Allow", it should be updated as "Prompt" or "Deny".
---it should be updated as "Deny" only.---

We double confirmed with the UX owner of control center, Aislinn and Philipp, and they both think(allow me to quote their words a little), the general structure of the permission prompts works pretty well for this prompt as well, even though it's different in a couple of important ways. We all agree to have copywriter's help to justify it by using different terms. 

> My concern about permission update is that it could surprise users if we
> don't notify this change to users?
> I think we can do some message refinement for different situations. For the
> case that existing a default handler "mail.yahoo.com", we probably can show
> the message "Would you like to change "mail.yahoo.com" to "mail.google.com"
> to be the default application to open all email links?" instead of the
> original one.

We don't think it'll be a surprise to users since it's user's decision and we can have copywriter fix the language. However, we agree it's a good idea to prompt differently on different scenarios to ease the concern. We'll update the spec to include the design.   

> After check with Morpheus, the handler will not be added into possible
> handlers after clicking "Manage Application". Nothing will be changed on the
> original page, user still needs to select "Always Allow" or "Never Allow" on
> the original page. 
> I agree Paolo's suggestion to left out this part from initial version,
> because I think the spec is still not well-defined. We can open a follow bug
> to trace the issue how to indicate user to "Application Panel" to get more
> information.
 
As I mentioned, we checked with the UX owner and we all think "Manage Application" can provide users more context. But, as Eden elaborated, it won't change any setting, it's just an entry point to instruct users the existence of content handler. Therefore, we think the prompt still works if there are consensus to left out this part from initial version.    

(In reply to :Paolo Amadini from comment #39)
> (In reply to Eden Chuang[:edenchuang] from comment #37)
> > Let me use a real case to figure out this design. [...]
> > Does this example show the design correctly?
> 
> It does, with one correction: "Handler Registration: Allowed" would not be
> shown in the Control Center because it is the default value for the
> permission.
> 
> In practice, you have described an edge case, because you have chosen an
> artificial example that does not happen in practice. In other words in your
> example IRCCloud wants to register for the "mailto" protocol, which is not
> the case in reality, and the user wants to register only one of the
> protocols, which isn't particularly useful. I don't think we'll have
> websites that want to register for two unrelated protocols, so even if this
> interaction looks contrived it will rarely happen, and we definitely
> shouldn't optimize for it, though it's good to discuss it.
> 
> In the example of two protocols, if you want to register the site then you
> get asked two times in a row, and you have to accept both. Ideally we would
> like to ask only one time for both "irc" and "ircs" if the site does the
> request at the same time, but at present the Privacy and Security team
> hasn't implemented this multiple requests feature.
> 
> If you do not want to use the site, you deny the first request, and don't
> see the other one. These are the two common use cases.

Thanks for Paolo's detailed explanation. Yes, we can just prompt multiple requests one by one for now since the implementation hasn't been there yet. But we can definitely discuss the detailed flow for these edge cases while we start to implement and also check the updates from control center.

Comment 41

[Eden Chuang[:edenchuang]]

(In reply to :Gijs Kruitbosch from comment #38)
> (In reply to Eden Chuang[:edenchuang] from comment #37)
> > (In reply to :Gijs Kruitbosch from comment #31)
> > > I have some questions:
> > > 
> > > - the design says "Whenever users set another website as the default
> > > application... the settings in control center will be updated accordingly" -
> > > but there doesn't seem to be a design for this case, as the app is clearly
> > > neither blocked nor allowed to handle *All* email links. 
> > 
> > I guess design concept is there is only one handler could be allowed at one
> > time, it also means the allowed one is the default handler. For the others,
> > they all should be blocked.
> > 
> > For the case that user allows a new website, the other websites' status
> > should be updated correspondingly.
> > If the original status is "Allow", it should be updated as "Prompt" or
> > "Deny". For other status, keep it as original. Permission update on other
> > websites is a very special usage of control center, I never seen that
> > before. I guess UX team want to let control center be another way to manage
> > application association. 
> 
> I still don't follow. If you click "Manage application", if I understand the
> spec we add the website's protocol handler as an option for the user to
> choose, but it is not made the default.

According to comment 40, I guess you probably misunderstand to spec.
Because comment 40 mentioned it will not be added into possible handlers if "Manage Application" is clicked.

> Likewise, my expectation was that if you have previously made one website's
> protocol handler the default, and now you go to a different one and make
> that the default for the same profile, the first website would remain in the
> list in about:preferences (but is no longer the default).

Yes, up to here your exception is correct. And it should be like you said.

> 
> Are you saying that "in the about:preferences list but not the default"
> should correspond to not allowing (always block) the use of
> registerProtocolHandler by that website? Because that seems wrong - there
> would be no feedback if the user clicked a button on the website to
> associate a protocol with the same website, because doing so would be
> blocked. Firefox would look broken.

I think all websites can be classed into followings

1. The default handler of the specific protocol
   The permission status for the protocol is "Allow".
   The website is in the list of about:preference.
   Don't prompt user when he visits the website again.

2. The denied handler of the specific protocol
   The permission status for the protocol is "Deny"
   The website is not in the list of about:preference.
   Don't prompt user when he visits the website again.

3. The possible handler of the protocol
   The permission status for the protocol is "Prompt"
   The website is in the list of about:preference.
   Prompt user when he visits the website again.

   But UX team said the status should be "Deny" and don't prompt user when he visits again. I still not understand what the purpose the design is. 

4. Unknown for the protocol
   For the website never call registerProtocolHandler() or is the first time visited.
   The permission status for the protocol is "Unknown", and the permission will not be shown on control center.
   The website is not in the list of about:preference.
   Prompt user if registerProtocolHandler() is called.

> 
> I guess the correct state for the control center would be to just not list a
> permission, and prompt again on that site if registerProtocolHandler is
> called again, or something?

That's why I said the status of the website need to updated as "Prompt", such that we can prompt to user when user visits the original default handler website. 

> 
> > Morpheus, could you give comments and explain the permission update design
> > concept?
> > 
> > My concern about permission update is that it could surprise users if we
> > don't notify this change to users?
> > I think we can do some message refinement for different situations. For the
> > case that existing a default handler "mail.yahoo.com", we probably can show
> > the message "Would you like to change "mail.yahoo.com" to "mail.google.com"
> > to be the default application to open all email links?" instead of the
> > original one.
> 
> This is a good idea, I think.
> 
> > > There also doesn't
> > > seem to be a design for the control center in the case where the user clicks
> > > "manage application" (which implicitly accepts adding the app as a possible
> > > handler without making it the default handler), where you end up in a
> > > similar situation.
> > > 
> > > - aren't we violating the principle of "least surprise" when clicking
> > > "Manage Application" silently adds the application in question as an option
> > > for that protocol?
> > > 
> > 
> > After check with Morpheus, the handler will not be added into possible
> > handlers after clicking "Manage Application". Nothing will be changed on the
> > original page, user still needs to select "Always Allow" or "Never Allow" on
> > the original page. 
> 
> I don't understand what would be the point of the link to "Manage
> Application" in that case. I also believe this doesn't match the current
> spec, which says "link the page to protocol handler in Settings".

I also feel strange that you open the "application panel" from the website, but you don't see it in the list. I think it needs be discussed more. I also didn't get the design purpose in comment 40.

> 
> > I agree Paolo's suggestion to left out this part from initial version,
> > because I think the spec is still not well-defined. We can open a follow bug
> > to trace the issue how to indicate user to "Application Panel" to get more
> > information.
> 
> If we are comfortable with making it hard/impossible for the user to add the
> handler but not make it the default, this is an option.

I think "Manage Application" links and permission update mechanism need to be discussed in detail.
But I think we are all agree that we can retire the original notification bar by using control center, is it right?

If it is right, to make this feature go forward. I will suggest separate the implementation into many steps.
The first step, we move prompt from the notification bar to control center and add the never ask("Never Allow") feature. At the same time we can discuss "Manage Application" and permission update in detail in the following bugs.

The second step, we can implement permission update and "Manage Application" link if we decide to implement it after discussion.

> 
> > > - which protocols are we supporting? The text says "email" everywhere, not
> > > "mailto", and so there must be some kind of mapping.
> > > 
> > > - if we have a mapping, maybe we should use clearer icons as well? The
> > > little square with an arrow isn't very "mail"-y to me. Maybe some version of
> > > our existing email link icon or some other use of the 'envelope' idiom would
> > > be more appropriate?
> > > 
> > 
> > Yes, there is a string mapping for some special protocols, such as mailto
> > and webcal, and a general message for other protocols. The message "Would
> > you like ... to open all the email links?" is copied form current release
> > version notification bar. Of course, we can do some message refinement to
> > make it be more clear. The clear icon is a good suggestion, I will reflect
> > it to UX.
> > I think UX team can refine the message and icon at the same time with
> > implementation, so this shouldn't block the code implementation, right?
> 
> Sure.
> 
> > (In reply to :Paolo Amadini from comment #34)
> > 
> > > I think the actual permission should be a single one for all schemes, and just be used to block the
> > > website from asking again.
> > Let me use a real case to figure out this design.
> 
> This is tricky to get right... :-(

No, I just trying to understand what the design Paolo mentioned is, because the Paolo's design is different with UX team gave. Permission and protocol association is one-many mapping in Paolo's design, but it is one-one mapping in UX design.

Comment 42

[:Gijs (he/him)]

(In reply to Eden Chuang[:edenchuang] from comment #41)
> (In reply to :Gijs Kruitbosch from comment #38)
> > (In reply to Eden Chuang[:edenchuang] from comment #37)
> > > (In reply to :Gijs Kruitbosch from comment #31)
> > > > I have some questions:
> > > > 
> > > > - the design says "Whenever users set another website as the default
> > > > application... the settings in control center will be updated accordingly" -
> > > > but there doesn't seem to be a design for this case, as the app is clearly
> > > > neither blocked nor allowed to handle *All* email links. 
> > > 
> > > I guess design concept is there is only one handler could be allowed at one
> > > time, it also means the allowed one is the default handler. For the others,
> > > they all should be blocked.
> > > 
> > > For the case that user allows a new website, the other websites' status
> > > should be updated correspondingly.
> > > If the original status is "Allow", it should be updated as "Prompt" or
> > > "Deny". For other status, keep it as original. Permission update on other
> > > websites is a very special usage of control center, I never seen that
> > > before. I guess UX team want to let control center be another way to manage
> > > application association. 
> > 
> > I still don't follow. If you click "Manage application", if I understand the
> > spec we add the website's protocol handler as an option for the user to
> > choose, but it is not made the default.
> 
> According to comment 40, I guess you probably misunderstand to spec.
> Because comment 40 mentioned it will not be added into possible handlers if
> "Manage Application" is clicked.

If I "misunderstand" the spec, then I think this is because the spec is clear as mud.

The spec *specifically says*, with a little line drawn to the "Manage application" link: "Open a new tab and link the page to protocol handler in Settings". How does "link the page to protocol handler in Settings" not mean "add the handler from this page to the list of possible handlers in Settings (by implication: but don't make it the default because the user didn't say 'Always Allow')"?

If that wasn't what the UX team that made the spec intends, maybe they should update the spec to be clearer. But as you note further down in your reply, it would be quite strange to have the link and then not show the item in the list...

> > Are you saying that "in the about:preferences list but not the default"
> > should correspond to not allowing (always block) the use of
> > registerProtocolHandler by that website? Because that seems wrong - there
> > would be no feedback if the user clicked a button on the website to
> > associate a protocol with the same website, because doing so would be
> > blocked. Firefox would look broken.
> 
> I think all websites can be classed into followings
> 
> 1. The default handler of the specific protocol
>    The permission status for the protocol is "Allow".
>    The website is in the list of about:preference.
>    Don't prompt user when he visits the website again.
> 
> 2. The denied handler of the specific protocol
>    The permission status for the protocol is "Deny"
>    The website is not in the list of about:preference.
>    Don't prompt user when he visits the website again.
> 
> 3. The possible handler of the protocol
>    The permission status for the protocol is "Prompt"
>    The website is in the list of about:preference.
>    Prompt user when he visits the website again.
> 
>    But UX team said the status should be "Deny" and don't prompt user when
> he visits again. I still not understand what the purpose the design is. 

In all of 1-3, I assume "prompt when the user visits the website again" means "prompt when the user visits the website and the website calls registerProtocolHandler", right? Not just any time we visit the website...

> 4. Unknown for the protocol
>    For the website never call registerProtocolHandler() or is the first time
> visited.
>    The permission status for the protocol is "Unknown", and the permission
> will not be shown on control center.
>    The website is not in the list of about:preference.
>    Prompt user if registerProtocolHandler() is called.
> 
> > 
> > I guess the correct state for the control center would be to just not list a
> > permission, and prompt again on that site if registerProtocolHandler is
> > called again, or something?
> 
> That's why I said the status of the website need to updated as "Prompt",
> such that we can prompt to user when user visits the original default
> handler website. 

Right, so my question is what we do for case 3. I don't know why we would need to treat 3 differently from 4 as far as the permissions state is concerned - "prompt" is the same as "no permission listed" (so clear either Deny/Allow state if present, no icon in the URL bar, we'll show a prompt if the page calls registerProtocolHandler but not otherwise). We just need to ensure that the handler shows up in the list of allowed applications in about:preferences.

If both of us are confused about this, then maybe Morpheus can clarify.

> > > I agree Paolo's suggestion to left out this part from initial version,
> > > because I think the spec is still not well-defined. We can open a follow bug
> > > to trace the issue how to indicate user to "Application Panel" to get more
> > > information.
> > 
> > If we are comfortable with making it hard/impossible for the user to add the
> > handler but not make it the default, this is an option.
> 
> I think "Manage Application" links and permission update mechanism need to
> be discussed in detail.
> But I think we are all agree that we can retire the original notification
> bar by using control center, is it right?

Yes.

> If it is right, to make this feature go forward. I will suggest separate the
> implementation into many steps.
> The first step, we move prompt from the notification bar to control center
> and add the never ask("Never Allow") feature. At the same time we can
> discuss "Manage Application" and permission update in detail in the
> following bugs.

Well, the state that I thought you would end up in through "manage application" ((3) in your list) you can also reach by simply allowing 2 pages (mail.yahoo and mail.google, for instance). So ignoring "Manage Application" doesn't really help. What happens with the first page you allowed in a prompt, that was superseded by the second page? There should be clarity about what this state looks like for the URL bar, the permissions center, what happens if a page calls registerProtocolHandler, and what happens in the preferences.

> > > > - which protocols are we supporting? The text says "email" everywhere, not
> > > > "mailto", and so there must be some kind of mapping.
> > > > 
> > > > - if we have a mapping, maybe we should use clearer icons as well? The
> > > > little square with an arrow isn't very "mail"-y to me. Maybe some version of
> > > > our existing email link icon or some other use of the 'envelope' idiom would
> > > > be more appropriate?
> > > > 
> > > 
> > > Yes, there is a string mapping for some special protocols, such as mailto
> > > and webcal, and a general message for other protocols. The message "Would
> > > you like ... to open all the email links?" is copied form current release
> > > version notification bar. Of course, we can do some message refinement to
> > > make it be more clear. The clear icon is a good suggestion, I will reflect
> > > it to UX.
> > > I think UX team can refine the message and icon at the same time with
> > > implementation, so this shouldn't block the code implementation, right?
> > 
> > Sure.
> > 
> > > (In reply to :Paolo Amadini from comment #34)
> > > 
> > > > I think the actual permission should be a single one for all schemes, and just be used to block the
> > > > website from asking again.
> > > Let me use a real case to figure out this design.
> > 
> > This is tricky to get right... :-(
> 
> No, I just trying to understand what the design Paolo mentioned is, because
> the Paolo's design is different with UX team gave. Permission and protocol
> association is one-many mapping in Paolo's design, but it is one-one mapping
> in UX design.

I think it isn't possible to have a fully 1:1 mapping between "is the page listed as "always allow" for *any* registerProtocolHandler and "is the page the default handler for protocol X"

A page can request to handle multiple protocols. It's pretty plausible that there would be pages that request multiple protocols (e.g. irccloud using both irc: and ircs) and one of them gets replaced later on by some other page (some other irc client only requests irc:). Now what happens when you re-visit irccloud and it calls registerProtocolHandler? Do we silently update the irc: association without notifying the user? That doesn't sound very good.

Comment 43

[:Paolo Amadini]

(In reply to :Gijs Kruitbosch from comment #42)
> Now what happens when you
> re-visit irccloud and it calls registerProtocolHandler? Do we silently
> update the irc: association without notifying the user? That doesn't sound
> very good.

From what I understand, we would ask again, because the user did not deny the permission the first time. We don't show a prompt in case the site is already the default handler OR the permission is currently blocked. There isn't a case where the permission is stored in the database as explicitly allow, because allow is the default state.

Comment 44

[Eden Chuang[:edenchuang]]

 > If I "misunderstand" the spec, then I think this is because the spec is
> clear as mud.
> 
> The spec *specifically says*, with a little line drawn to the "Manage
> application" link: "Open a new tab and link the page to protocol handler in
> Settings". How does "link the page to protocol handler in Settings" not mean
> "add the handler from this page to the list of possible handlers in Settings
> (by implication: but don't make it the default because the user didn't say
> 'Always Allow')"?
> 
> If that wasn't what the UX team that made the spec intends, maybe they
> should update the spec to be clearer. But as you note further down in your
> reply, it would be quite strange to have the link and then not show the item
> in the list...
> 

Sorry, I can't explain why the design is different with your understanding. But I agree that this part of spec should be more clear and define what will happen when user clicks the "Manage Application". 

let me quote the comment 40 replied by Morpheus.

> We checked with the UX owner and we all think "Manage Application" can provide users more context. But,
> as Eden elaborated, it won't change any setting, it's just an entry point to instruct users the existence
> of content handler. Therefore, we think the prompt still works if there are consensus to left out this 
> part from initial version.

Although I feel the design is strange, I am not a UX expert. I guess there must exist some reasons UX design this behavior. 

Morpheus, maybe you can explain why you think opening "Application Panel" in the new tab without the prompted website provides users more context. 

> > 
> > I think all websites can be classed into followings
> > 
> > 1. The default handler of the specific protocol
> >    The permission status for the protocol is "Allow".
> >    The website is in the list of about:preference.
> >    Don't prompt user when he visits the website again.
> > 
> > 2. The denied handler of the specific protocol
> >    The permission status for the protocol is "Deny"
> >    The website is not in the list of about:preference.
> >    Don't prompt user when he visits the website again.
> > 
> > 3. The possible handler of the protocol
> >    The permission status for the protocol is "Prompt"
> >    The website is in the list of about:preference.
> >    Prompt user when he visits the website again.
> > 
> >    But UX team said the status should be "Deny" and don't prompt user when
> > he visits again. I still not understand what the purpose the design is. 
> 
> In all of 1-3, I assume "prompt when the user visits the website again"
> means "prompt when the user visits the website and the website calls
> registerProtocolHandler", right? Not just any time we visit the website...
> 

Yes, only prompt when the user visits the website and the website calls registerProtocolHandler().

> > 4. Unknown for the protocol
> >    For the website never call registerProtocolHandler() or is the first time
> > visited.
> >    The permission status for the protocol is "Unknown", and the permission
> > will not be shown on control center.
> >    The website is not in the list of about:preference.
> >    Prompt user if registerProtocolHandler() is called.
> > 

> 
> Right, so my question is what we do for case 3. I don't know why we would
> need to treat 3 differently from 4 as far as the permissions state is
> concerned - "prompt" is the same as "no permission listed" (so clear either
> Deny/Allow state if present, no icon in the URL bar, we'll show a prompt if
> the page calls registerProtocolHandler but not otherwise). We just need to
> ensure that the handler shows up in the list of allowed applications in
> about:preferences.

Yes, I think we could treat case 3 and case 4 with the same behavior, it means prompt if the website call registerProtocolHandler, and make the case 3 handlers are in the list of about:preference. But it must be under the situation that the case 3 status is not updated to "Allow" or "Deny". Unfortunately, UX said it is updated to "Deny".

Morpheus could you explain why the original default handler need to be update as "Deny"

> 
> If both of us are confused about this, then maybe Morpheus can clarify.
> 
> > 
> > I think "Manage Application" links and permission update mechanism need to
> > be discussed in detail.
> > But I think we are all agree that we can retire the original notification
> > bar by using control center, is it right?
> 
> Yes.
> 
> > If it is right, to make this feature go forward. I will suggest separate the
> > implementation into many steps.
> > The first step, we move prompt from the notification bar to control center
> > and add the never ask("Never Allow") feature. At the same time we can
> > discuss "Manage Application" and permission update in detail in the
> > following bugs.
> 
> Well, the state that I thought you would end up in through "manage
> application" ((3) in your list) you can also reach by simply allowing 2
> pages (mail.yahoo and mail.google, for instance). So ignoring "Manage
> Application" doesn't really help. What happens with the first page you
> allowed in a prompt, that was superseded by the second page? There should be
> clarity about what this state looks like for the URL bar, the permissions
> center, what happens if a page calls registerProtocolHandler, and what
> happens in the preferences.

No, in the first step, I just want implement "Always Allow" for adding to possible handlers list in about:preference but not the default handler, and "Never Allow" for never prompt when visiting the website in the future even it calls registerProtocolHandler() and don't save it in list of about:preference.
So, in your example, both permission status for mail.yahoo and mail.google are "Allow", and both they are in the list of about:preference but no one of them is the default handler. 

"Always Allow" for set as default and "Manage Application" would be in the second step if we conclude to decide to do in the follow discussion. 

> > 
> > No, I just trying to understand what the design Paolo mentioned is, because
> > the Paolo's design is different with UX team gave. Permission and protocol
> > association is one-many mapping in Paolo's design, but it is one-one mapping
> > in UX design.
> 
> I think it isn't possible to have a fully 1:1 mapping between "is the page
> listed as "always allow" for *any* registerProtocolHandler and "is the page
> the default handler for protocol X"
> 

1:1 mapping is between protocol and its permission to be the default handler. 
It means for each website, you have one permission to one protocol while one registerProtocolHandler is called.

1:many mapping is between one permission for all registerProtocolHandler calls. In Paolo's design, once the permission is set as "Deny" no any registerProtocolHandler calls invoke the prompt to user. 

So I said that the design between UX purposed and Paolo mentioned are different. 

> A page can request to handle multiple protocols. It's pretty plausible that
> there would be pages that request multiple protocols (e.g. irccloud using
> both irc: and ircs) and one of them gets replaced later on by some other
> page (some other irc client only requests irc:). Now what happens when you
> re-visit irccloud and it calls registerProtocolHandler? Do we silently
> update the irc: association without notifying the user? That doesn't sound
> very good.

Like Paolo's reply in comment 43, we will prompt again.

Comment 45

[:Gijs (he/him)]

(In reply to Eden Chuang[:edenchuang] from comment #44)
> "Always Allow" for set as default and "Manage Application" would be in the
> second step if we conclude to decide to do in the follow discussion. 

I mean, we can start doing what you propose, but I think there won't be a real gain for users until/unless we do the "set as default" 'for' the user (ie "Always allow" triggers setting the protocol handler in question as the default for that protocol).

Comment 46

[Morpheus Chen [:morpheus] UX]

Hi Gijs,

Based on the comments above, I noticed some of the misunderstandings can be attributed to the ambiguity in meaning. Sorry that I didn't facilitate the wording which might result in misleading the purpose and attempts of the UX design. We'll keep consulting the copywriter to prevent any of the confusions. From UX perspective, we do value all feedbacks from the different point of views on the UX design. Therefore, we still look forward to having your thoughts or concerns to consolidate the UX spec and make it more self-explanatory. 

Here I'm going to provide more context to the major 2 arguments which triggers tons of discussions above. Before that, I want to shortly address the overall design concept and intents behind the flow. The main goal to redesign the notification bar is to simplify the concept and process of protocol handler to educate general users what protocol is and how to handle these protocols in about: preferences. Therefore, we rephrase the language to remove the ambiguity of "add application" and replace with the concept of "set as default", which is the same as what we provide in about: preferences. (we definitely do ask copywriter for help after feasibility checking.)  To continue the intention and promote the protocol handler feature, we put a link on the door hanger to lead users to about: preferences page which provides instruction and mechanism in our new design and also helper page link in the current design. As the UX spec said, please noted that the link here only offers informative purpose in the new tab since user still needs to make a decision to the door hanger in the original tab. (Redesigned door hanger won't be dismissed unless the user makes a decision. Please find Aislinn's spec for more details.)

As for the first arguments, as Gijs mentioned:

> Right, so my question is what we do for case 3. I don't know why we would
> need to treat 3 differently from 4 as far as the permissions state is
> concerned - "prompt" is the same as "no permission listed" (so clear either
> Deny/Allow state if present, no icon in the URL bar, we'll show a prompt if
> the page calls registerProtocolHandler but not otherwise). We just need to
> ensure that the handler shows up in the list of allowed applications in
> about: preferences.

Let's take Gmail and Yahoo mail as an example. The use case is that user sets Gmail as default handler first, then visit Yahoo mail and set Yahoo mail as default handler to replace Gmail. There are several reasons we treat case 3 differently. First, to alleviate the annoyance caused by frequent prompts. We believe users deciding to have Yahoo mail as default application is a pretty clear intention to replace no matter the previous decision is, therefore, prompting again when visiting the site again could annoy users. Besides, users can always change the settings in about: preferences any time which is what we want to promote. Second, as you agreed in comment 38, prompting different messages on different scenarios can help ease the concern. For example, "Would you like to allow mail.yahoo.com to replace mail.google.com to be the default application to open all email links?" We'll ask copywriter for help on providing a crystal clear language to tell users the consequence if there has been a default handler chosen before. Third, the address bar should display an icon with a slash mark (/) through it to indicate blocked when users visit Gmail again.  This can provide users consistent understanding of the context. 

For the second argument, as Gijs said:

> If I "misunderstand" the spec, then I think this is because the spec is
> clear as mud.
> 
> The spec *specifically says*, with a little line drawn to the "Manage
> application" link: "Open a new tab and link the page to protocol handler in
> Settings". How does "link the page to protocol handler in Settings" not mean
> "add the handler from this page to the list of possible handlers in Settings
> (by implication: but don't make it the default because the user didn't say
> 'Always Allow')"?
> 
> If that wasn't what the UX team that made the spec intends, maybe they
> should update the spec to be clearer. But as you note further down in your
> reply, it would be quite strange to have the link and then not show the item
> in the list...  

The intention to have an informative link here is what I elaborate before. We believe attaching the about: preferences can not only provide more context but also promote the feature. However, the term of "manage application" seems not perfectly convey what we intended to address. We'll consult copywriter for a better term, such as "learn more", etc. As for the conclusion, so far we have three solutions on the table based on the discussion above 1) leading users to about: preferences page as informative material; 2) simply remove the link which seems the final consensus we had; 3) lead users to the helper page. Since the link plays a role of "nice-to-have" instead of "must-to-have", we are opened to the 3 solutions if there are any schedule or feasibility concerns.

Comment 47

[:Gijs (he/him)]

OK. I think we should leave out this "Manage Applications" link for now, but "Always allow" *should* set a page as the default (different from what happens now). I think that's fine for v1 and we can worry about the extra link in some other bug.


(In reply to Morpheus Chen [:morpheus]  UX from comment #46)
> As the UX spec said, please
> noted that the link here only offers informative purpose in the new tab
> since user still needs to make a decision to the door hanger in the original
> tab. (Redesigned door hanger won't be dismissed unless the user makes a
> decision. Please find Aislinn's spec for more details.)

I don't see a comment like this in the spec. You also haven't linked to it. I'm assuming you're referring to https://mozilla.invisionapp.com/share/4Y6ZZH1E8#/screens/162255220 and the next page (attached as screenshots to this bug). Is there some other document I should be looking at?

> > Right, so my question is what we do for case 3. I don't know why we would
> > need to treat 3 differently from 4 as far as the permissions state is
> > concerned - "prompt" is the same as "no permission listed" (so clear either
> > Deny/Allow state if present, no icon in the URL bar, we'll show a prompt if
> > the page calls registerProtocolHandler but not otherwise). We just need to
> > ensure that the handler shows up in the list of allowed applications in
> > about: preferences.
> 
> Let's take Gmail and Yahoo mail as an example. The use case is that user
> sets Gmail as default handler first, then visit Yahoo mail and set Yahoo
> mail as default handler to replace Gmail. There are several reasons we treat
> case 3 differently. First, to alleviate the annoyance caused by frequent
> prompts. We believe users deciding to have Yahoo mail as default application
> is a pretty clear intention to replace no matter the previous decision is,
> therefore, prompting again when visiting the site again could annoy users.

It seems to me like prompting repeatedly / user annoyance should be tackled in a different way, e.g. by using popup blocker mechanisms to prevent registerProtocolHandler from doing anything if not the result of a click, just like e.g. the desktop notification permission.

Comment 48

[Morpheus Chen [:morpheus] UX]

Hi Gijs,

Thanks for your feedback, please find my comments below.

(In reply to :Gijs Kruitbosch from comment #47)
> OK. I think we should leave out this "Manage Applications" link for now, but
> "Always allow" *should* set a page as the default (different from what
> happens now). I think that's fine for v1 and we can worry about the extra
> link in some other bug.
 
From UX perspective, we totally agree your v1 proposal. As mentioned in comment 46, we can leave out the link to "manage application" for now since it'll be reviewed by copywriter, but we definitely need to have "always allow" implemented.
 
> (In reply to Morpheus Chen [:morpheus]  UX from comment #46)
> > As the UX spec said, please
> > noted that the link here only offers informative purpose in the new tab
> > since user still needs to make a decision to the door hanger in the original
> > tab. (Redesigned door hanger won't be dismissed unless the user makes a
> > decision. Please find Aislinn's spec for more details.)

You can find "Notification Design Updates" for more details in the second document I list below.
 
> I don't see a comment like this in the spec. You also haven't linked to it.
> I'm assuming you're referring to
> https://mozilla.invisionapp.com/share/4Y6ZZH1E8#/screens/162255220 and the
> next page (attached as screenshots to this bug). Is there some other
> document I should be looking at?

It's my bad, I assumed you had the access to these documents. The documents I am referring detail the new design of Control Center. You can find the link below and I'll put the references in the UX spec too. 

https://mozilla.invisionapp.com/share/DM3XGA6YE#/screens
https://docs.google.com/document/d/143nEfWfIvFZD2-pFqE8GD85inFjFIhE2J8QeegQufc0/edit?ts=57ab0423#heading=h.w77gtkxe50a5
 
> > > Right, so my question is what we do for case 3. I don't know why we would
> > > need to treat 3 differently from 4 as far as the permissions state is
> > > concerned - "prompt" is the same as "no permission listed" (so clear either
> > > Deny/Allow state if present, no icon in the URL bar, we'll show a prompt if
> > > the page calls registerProtocolHandler but not otherwise). We just need to
> > > ensure that the handler shows up in the list of allowed applications in
> > > about: preferences.
> > 
> > Let's take Gmail and Yahoo mail as an example. The use case is that user
> > sets Gmail as default handler first, then visit Yahoo mail and set Yahoo
> > mail as default handler to replace Gmail. There are several reasons we treat
> > case 3 differently. First, to alleviate the annoyance caused by frequent
> > prompts. We believe users deciding to have Yahoo mail as default application
> > is a pretty clear intention to replace no matter the previous decision is,
> > therefore, prompting again when visiting the site again could annoy users.
> 
> It seems to me like prompting repeatedly / user annoyance should be tackled
> in a different way, e.g. by using popup blocker mechanisms to prevent
> registerProtocolHandler from doing anything if not the result of a click,
> just like e.g. the desktop notification permission.

Since we've got the consensus on the current UX spec, I would suggest implementing v1 first. At the mean time, we can consider the "popup blocker mechanisms" as an enhancer and have a separate bug to discuss more since this might be relevant to other permissions. Would you mind elaborating more on your idea in the separate bug?

Eden, could you help file another bug for this idea? Much appreciated.

Comment 49

[:Gijs (he/him)]

(In reply to Morpheus Chen [:morpheus]  UX from comment #48)
> Hi Gijs,
> 
> Thanks for your feedback, please find my comments below.
> 
> (In reply to :Gijs Kruitbosch from comment #47)
> > OK. I think we should leave out this "Manage Applications" link for now, but
> > "Always allow" *should* set a page as the default (different from what
> > happens now). I think that's fine for v1 and we can worry about the extra
> > link in some other bug.
>  
> From UX perspective, we totally agree your v1 proposal. As mentioned in
> comment 46, we can leave out the link to "manage application" for now since
> it'll be reviewed by copywriter, but we definitely need to have "always
> allow" implemented.

Great.

> > (In reply to Morpheus Chen [:morpheus]  UX from comment #46)
> > > As the UX spec said, please
> > > noted that the link here only offers informative purpose in the new tab
> > > since user still needs to make a decision to the door hanger in the original
> > > tab. (Redesigned door hanger won't be dismissed unless the user makes a
> > > decision. Please find Aislinn's spec for more details.)
> 
> You can find "Notification Design Updates" for more details in the second
> document I list below.

I looked through this section but could not find what you meant. I searched for "protocol" (no hits) and "link" (for this explanatory link that opens a new tab - no relevant hits).

> > I don't see a comment like this in the spec. You also haven't linked to it.
> > I'm assuming you're referring to
> > https://mozilla.invisionapp.com/share/4Y6ZZH1E8#/screens/162255220 and the
> > next page (attached as screenshots to this bug). Is there some other
> > document I should be looking at?
> 
> It's my bad, I assumed you had the access to these documents. The documents
> I am referring detail the new design of Control Center. You can find the
> link below and I'll put the references in the UX spec too. 
> 
> https://mozilla.invisionapp.com/share/DM3XGA6YE#/screens
> https://docs.google.com/document/d/143nEfWfIvFZD2-
> pFqE8GD85inFjFIhE2J8QeegQufc0/edit?ts=57ab0423#heading=h.w77gtkxe50a5

Neither of these seem to have any references to the protocol handling work in this bug? They seem to be generic documents about the control center work in general, but in neither could I find anything about the explanatory link.

As noted earlier, IMO if we title the link "Manage Applications" or similar, and we open about:preferences with the 'applications' section in it open, the user would expect to be able to then change the applications that handle 'mail' (or whatever the page has requested) and the webpage that triggered the doorhanger should be in the list.

If, alternatively, we make the link text "Learn more" and make it link to somewhere on SUMO, that seems fine - but that achieves a different goal (user education vs. directly allowing the user to make detailed / more complex choices than "always allow"/"always deny"). Of course, the SUMO page could contain step-by-step instructions for how to get to the preferences and make changes, but that is less direct. Ultimately it depends what the goal of that link is.


> Since we've got the consensus on the current UX spec, I would suggest
> implementing v1 first. At the mean time, we can consider the "popup blocker
> mechanisms" as an enhancer and have a separate bug to discuss more since
> this might be relevant to other permissions. Would you mind elaborating more
> on your idea in the separate bug?

Sure.

Comment 50

[Morpheus Chen [:morpheus] UX]

Hi Gijs,

Please noted "manage application" won't be implemented for now. The content below is to answer you questions.

Yes, the documents I provided are generic UX spec of Control Center. As I mentioned, since protocol door hanger follows the mechanism of Control Center, we only detailed the general guideline and different rules, which we've consulted with Aislinn and Philipp and we all agreed with the design, in our UX spec. Therefore, the documents are for references in case anyone doesn't fully understand the redesigned Control Center. Besides, to show you the defined rule in the generic spec below which explains our design intention on the informative link:

If a user clicks anywhere on the page, the notification is NOT dismissed.

Since users must make a decision to dismiss the doorhanger in the web page tab, they'll be confused if they can find the web page that triggered the doorhanger in the list which provides the duplicate function as door hanger does. Anyway, the link is not defined in the generic document, but we all believe it can provide more context to users no matter where it leads, especially for a protocol which is a vague concept to general users. Therefore, I would suggest keeping the link to serve for informative purpose.

In addition, as far as I know, the generic document provides UX designers a guideline to follow when needing the tool to ask users for permissions. So it would be a tough work to keep collecting permission requests in the generic document for Aislinn. And that's why you can't find anything related to protocol door hanger in the document.

Comment 51

[Eden Chuang[:edenchuang]]

I open bug 1311940 for popup block mechanism of registerProtocolHandler and bug 1311943 for informative link support in prompted message.

I will start to implement the v1 spec and tracking implementation on this bug.

Comment 52

[Eden Chuang[:edenchuang]]

Attachment: Bug1270416_Improve_registerProtocolHandler_with_control_center.

Comment 53

[Eden Chuang[:edenchuang]]

Attachment: Bug1270416_Improve_registerProtocolHandler_with_permission_manager.

Hello Gijs

I wrote a patch which uses PermissionUI.jsm to prompt user and set the website as the default handler. I think we can have a discussion based on this patch in Hawaii. What do you think?

Comment 54

[:Gijs (he/him)]

Comment on attachment 8816068 [details] [diff] [review]
Bug1270416_Improve_registerProtocolHandler_with_permission_manager.

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

What do you want to talk about in hawaii? Do you think the patch is/was ready for review? Do you have questions? Meeting about this in Hawaii is mostly useful if there are concrete things you have questions about or that you need clarifying from me, unrelated to pure code review.

As for the patch, please submit the next version via mozreview, so that code moves are more obvious. Please also add either Paolo or florian as a reviewer for the permission ui use.

Please add tests for the next version of the patch.

::: browser/components/feeds/WebContentConverter.js
@@ +247,5 @@
> +    return this._principal;
> +  },
> +
> +  get permissionKey() {
> +    return "handler-registration-"+this._type;

Did you run ./mach eslint? I thought we turned on infix operator spacing rules. Anyway, nit: spaces around '+'.

@@ +279,5 @@
> +    handlerInfo.preferredApplicationHandler = handler;
> +    handlerInfo.preferredAction = Ci.nsIHandlerInfo.useHelperApp;
> +
> +    let hs = Cc["@mozilla.org/uriloader/handler-service;1"].
> +             getService(Ci.nsIHandlerService);

It's pretty odd that we're getting the handler info from one service and storing it in another. Is there no fetching API on nsIHandlerService, or storing API on nsIExtProtSvc?

@@ +292,5 @@
> +    while (handlersEnum.hasMoreElements()) {
> +      let modifiedHandler = handlersEnum.getNext().
> +                            QueryInterface(Ci.nsIWebHandlerApp);
> +      if (modifiedHandler) {
> +	let modifiedURI = Services.io.newURI(modifiedHandler.uriTemplate, null, null);

So this is generally odd... can't foo.com add a handler for mail.foo.com, thus causing the permission to be on foo.com but the handler to be for mail.foo.com? That's what I would expect. The current code stores the permission for this._principal, not this._uri which is used for the handler's URI template - so they could be different.

At what point do we check these permissions? I don't see any checks for them in the current code - is that in unmodified Utils.checkProtocolHandlerAllowed code?

It seems to me that it would make more sense to enumerate all the URIs that have a permission stored for the protocol using the permissions service, and revoke any permissions that aren't for the URI we're currently dealing with. That avoids the uri vs. uriTemplate ambiguity, as well as ensuring we revoke permissions for sites where the user has manually removed the handler using the preferences.

@@ +294,5 @@
> +                            QueryInterface(Ci.nsIWebHandlerApp);
> +      if (modifiedHandler) {
> +	let modifiedURI = Services.io.newURI(modifiedHandler.uriTemplate, null, null);
> +        let modifiedPrincipal = Services.scriptSecurityManager.
> +	    createCodebasePrincipal(modifiedURI, {});

Again, not convinced how this works with private browsing and containers

@@ +298,5 @@
> +	    createCodebasePrincipal(modifiedURI, {});
> +	let modifiedPermStatus = Services.perms.testExactPermissionFromPrincipal(
> +	                         modifiedPrincipal, this.permissionKey);
> +	if (modifiedPermStatus != Services.perms.DENY_ACTION &&
> +            modifiedHandler.name != this._title) {

So if two websites use the same title for their handler ("Mail client"), this won't revoke it? That seems wrong. Compare the URIs instead (uri.equalsExceptRef).

@@ +320,5 @@
> +                               permsServ.EXPIRE_NEVER);
> +  },
> +
> +  allow() {
> +    LOG("HandlerRegistrationRequest::allow()");

Is this really useful?

@@ +527,5 @@
>      Utils.checkProtocolHandlerAllowed(aProtocol, aURIString,
>                                        haveWindow ? aBrowserOrWindow : null);
> +    let handlerRegistrationRequest =
> +        new HandlerRegistrationRequest(browser,
> +                                       Services.scriptSecurityManager.createCodebasePrincipal(uri, {}),

Pretty sure we should be reusing a principal from somewhere that invoked this code rather than making it up as we go along. This won't have the right originAttributes, private browsing flags, etc.

In fact, should this prompt be supported in pb, or silently ignored? What happens if you register a protocol handler in one container (userContextId) - does it apply in the others? Etc.

::: browser/locales/en-US/chrome/browser/feeds/subscribe.properties
@@ +46,5 @@
>  feedSubscriptionVideoPodcast2=You can subscribe to this video podcast to receive updates when this content changes.
>  
>  # Protocol Handling
>  # "Add %appName (%appDomain) as an application for %protocolType links?"
> +replaceDefaultProtocolHandler=Would you like to change %S to %S (%S) to be the default application for %S links?

You should update the l10n comment above.

@@ +47,5 @@
>  
>  # Protocol Handling
>  # "Add %appName (%appDomain) as an application for %protocolType links?"
> +replaceDefaultProtocolHandler=Would you like to change %S to %S (%S) to be the default application for %S links?
> +setDefaultProtocolHandler=Set %S (%S) as the default application for %S links?

and add one for this string.

::: browser/modules/PermissionUI.jsm
@@ +78,5 @@
>                   .createBundle('chrome://browser/locale/browser.properties');
>  });
>  
> +function LOG(aMsg) {
> +  dump("@ PermissionUI: " + aMsg + "\n");

Leftover debug code?

@@ +375,5 @@
>    get browser() {
>      // In the e10s-case, the <xul:browser> will be at request.element.
>      // In the single-process case, we have to use some XPCOM incantations
>      // to resolve to the <xul:browser>.
> +

?

@@ +653,5 @@
> +      if (defaultHandler.name != this.request.title) {
> +        let originalHandlerName;
> +        if (defaultHandler instanceof Ci.nsIWebHandlerApp) {
> +	  let originalURI = Services.io.newURI(defaultHandler.uriTemplate, null, null);
> +	  originalHandlerName = defaultHandler.name + " (" + originalURI.host +")";

This should be a localizable string.

@@ +659,5 @@
> +	  originalHandlerName = defaultHandler.name;
> +	}
> +        let perms = [originalHandlerName,
> +	             this.request.title,
> +                     this.request.uri.host,

This can throw.

More generally, I think this should show the hostname from the principal instead of from the URI (so that if evil.com opens a data: uri in a new window that requests this, we show evil.com and not nothing). That might need changing in more places, if you copied this from elsewhere, in which case please file a bug on this.

@@ +660,5 @@
> +	}
> +        let perms = [originalHandlerName,
> +	             this.request.title,
> +                     this.request.uri.host,
> +                     this.request.type];

Nit: more typical style is:

let perms = [
  ...,
  ...,
];

Comment 55

[:Gijs (he/him)]

Also, based on the quoting below as supplied by splinter, it looks as if you're mixing tabs and spaces. If so, please don't. :-)

Comment 56

[Eden Chuang[:edenchuang]]

(In reply to :Gijs Kruitbosch from comment #54)
> Comment on attachment 8816068 [details] [diff] [review]
> Bug1270416_Improve_registerProtocolHandler_with_permission_manager.
> 
> Review of attachment 8816068 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> What do you want to talk about in hawaii? Do you think the patch is/was
> ready for review? Do you have questions? Meeting about this in Hawaii is
> mostly useful if there are concrete things you have questions about or that
> you need clarifying from me, unrelated to pure code review.
> 
> As for the patch, please submit the next version via mozreview, so that code
> moves are more obvious. Please also add either Paolo or florian as a
> reviewer for the permission ui use.
> 
> Please add tests for the next version of the patch.
> 
> ::: browser/components/feeds/WebContentConverter.js
> @@ +247,5 @@
> > +    return this._principal;
> > +  },
> > +
> > +  get permissionKey() {
> > +    return "handler-registration-"+this._type;
> 
> Did you run ./mach eslint? I thought we turned on infix operator spacing
> rules. Anyway, nit: spaces around '+'.
> 
> @@ +279,5 @@
> > +    handlerInfo.preferredApplicationHandler = handler;
> > +    handlerInfo.preferredAction = Ci.nsIHandlerInfo.useHelperApp;
> > +
> > +    let hs = Cc["@mozilla.org/uriloader/handler-service;1"].
> > +             getService(Ci.nsIHandlerService);
> 
> It's pretty odd that we're getting the handler info from one service and
> storing it in another. Is there no fetching API on nsIHandlerService, or
> storing API on nsIExtProtSvc?
> 

nsIExternalProtocolService::getProtocolHandlerInfo will try to get protocol handler from OS setting first, then if it can get the protocol handler information from our database, mimeTypes.rdf, through nsIHandlerService, and it drops the information from OS. 

https://dxr.mozilla.org/mozilla-central/source/uriloader/exthandler/nsExternalHelperAppService.cpp#1155

nsIHandlerService is the interface used to interact with our database, so if we wants to save the handler information in our database, we should use nsIHandlerService. 

That's the reason we get and save handler information from different handler service component.

In fact, I just reuse the code from original handler registration in WebContentConverter.js 

https://dxr.mozilla.org/mozilla-central/source/browser/components/feeds/WebContentConverter.js#440

> @@ +292,5 @@
> > +    while (handlersEnum.hasMoreElements()) {
> > +      let modifiedHandler = handlersEnum.getNext().
> > +                            QueryInterface(Ci.nsIWebHandlerApp);
> > +      if (modifiedHandler) {
> > +	let modifiedURI = Services.io.newURI(modifiedHandler.uriTemplate, null, null);
> 
> So this is generally odd... can't foo.com add a handler for mail.foo.com,
> thus causing the permission to be on foo.com but the handler to be for
> mail.foo.com? That's what I would expect. The current code stores the
> permission for this._principal, not this._uri which is used for the
> handler's URI template - so they could be different.
> 

might be we can use testExactPermission() and addPermission() to replace testExactPermissionFromPrincipal() and addPermissionFromPrincipal(). But in fact testExactPermission() and addPermission() are a wrapper function of testExactPermissionPrincipal and addPermissionFromPrincipal(), I am not sure is this you want?

> At what point do we check these permissions? I don't see any checks for them
> in the current code - is that in unmodified
> Utils.checkProtocolHandlerAllowed code?

No, we check the permission status when prompt() before the pop-up notification is shown.

https://dxr.mozilla.org/mozilla-central/source/browser/modules/PermissionUI.jsm#275

This is original code of PermissionUI.jsm. 

> 
> It seems to me that it would make more sense to enumerate all the URIs that
> have a permission stored for the protocol using the permissions service, and
> revoke any permissions that aren't for the URI we're currently dealing with.
> That avoids the uri vs. uriTemplate ambiguity, as well as ensuring we revoke
> permissions for sites where the user has manually removed the handler using
> the preferences.

Yes, just like you mentioned, we can traverse all permissions for the type through the permissions service, and revoke the uri that aren't we are dealing with. 

But here is problem that we pre-load some protocol handlers, such as mibbit for irc, gmail, yahoo mail for mailto, and we don't give a permission when we pre-load it, we can't get the handler's permission from permissions service. To resolve this problem, I traverse the all possible handlers and try to create a permission for them.

> 
> @@ +294,5 @@
> > +                            QueryInterface(Ci.nsIWebHandlerApp);
> > +      if (modifiedHandler) {
> > +	let modifiedURI = Services.io.newURI(modifiedHandler.uriTemplate, null, null);
> > +        let modifiedPrincipal = Services.scriptSecurityManager.
> > +	    createCodebasePrincipal(modifiedURI, {});
> 
> Again, not convinced how this works with private browsing and containers

In fact, private browsing is not allow to register any content handler. Please see

https://dxr.mozilla.org/mozilla-central/source/browser/components/feeds/WebContentConverter.js#413

if user is in pb, the API will do nothing and return out directly.

> 
> @@ +298,5 @@
> > +	    createCodebasePrincipal(modifiedURI, {});
> > +	let modifiedPermStatus = Services.perms.testExactPermissionFromPrincipal(
> > +	                         modifiedPrincipal, this.permissionKey);
> > +	if (modifiedPermStatus != Services.perms.DENY_ACTION &&
> > +            modifiedHandler.name != this._title) {
> 
> So if two websites use the same title for their handler ("Mail client"),
> this won't revoke it? That seems wrong. Compare the URIs instead
> (uri.equalsExceptRef).
> 
> @@ +320,5 @@
> > +                               permsServ.EXPIRE_NEVER);
> > +  },
> > +
> > +  allow() {
> > +    LOG("HandlerRegistrationRequest::allow()");
> 
> Is this really useful?
> 
> @@ +527,5 @@
> >      Utils.checkProtocolHandlerAllowed(aProtocol, aURIString,
> >                                        haveWindow ? aBrowserOrWindow : null);
> > +    let handlerRegistrationRequest =
> > +        new HandlerRegistrationRequest(browser,
> > +                                       Services.scriptSecurityManager.createCodebasePrincipal(uri, {}),
> 
> Pretty sure we should be reusing a principal from somewhere that invoked
> this code rather than making it up as we go along. This won't have the right
> originAttributes, private browsing flags, etc.
> 

PermissionUI needs a principal to get the permission status, when prompt() is called. 

https://dxr.mozilla.org/mozilla-central/source/browser/modules/PermissionUI.jsm#275

Of course, we can use uri to replace principal by using testExactPerimssion()

https://dxr.mozilla.org/mozilla-central/source/netwerk/base/nsIPermissionManager.idl#188

Howerver, in fact, the implementation of testExactPermission also uses the passed in uri to generate a principal and then call testExactPermissionFromPrincipal()

https://dxr.mozilla.org/mozilla-central/source/extensions/cookie/nsPermissionManager.cpp#1945

https://dxr.mozilla.org/mozilla-central/source/extensions/cookie/nsPermissionManager.cpp#164

I am not sure could we get a principal from somewhere, but I will try it.

> In fact, should this prompt be supported in pb, or silently ignored? What
> happens if you register a protocol handler in one container (userContextId)
> - does it apply in the others? Etc.
> 
> ::: browser/locales/en-US/chrome/browser/feeds/subscribe.properties
> @@ +46,5 @@
> >  feedSubscriptionVideoPodcast2=You can subscribe to this video podcast to receive updates when this content changes.
> >  
> >  # Protocol Handling
> >  # "Add %appName (%appDomain) as an application for %protocolType links?"
> > +replaceDefaultProtocolHandler=Would you like to change %S to %S (%S) to be the default application for %S links?
> 
> You should update the l10n comment above.
> 
> @@ +47,5 @@
> >  
> >  # Protocol Handling
> >  # "Add %appName (%appDomain) as an application for %protocolType links?"
> > +replaceDefaultProtocolHandler=Would you like to change %S to %S (%S) to be the default application for %S links?
> > +setDefaultProtocolHandler=Set %S (%S) as the default application for %S links?
> 
> and add one for this string.
> 

I will add l10n comment for these strings in next version.

> ::: browser/modules/PermissionUI.jsm
> @@ +78,5 @@
> >                   .createBundle('chrome://browser/locale/browser.properties');
> >  });
> >  
> > +function LOG(aMsg) {
> > +  dump("@ PermissionUI: " + aMsg + "\n");
> 
> Leftover debug code?
> 
> @@ +375,5 @@
> >    get browser() {
> >      // In the e10s-case, the <xul:browser> will be at request.element.
> >      // In the single-process case, we have to use some XPCOM incantations
> >      // to resolve to the <xul:browser>.
> > +
> 
> ?
> 
> @@ +653,5 @@
> > +      if (defaultHandler.name != this.request.title) {
> > +        let originalHandlerName;
> > +        if (defaultHandler instanceof Ci.nsIWebHandlerApp) {
> > +	  let originalURI = Services.io.newURI(defaultHandler.uriTemplate, null, null);
> > +	  originalHandlerName = defaultHandler.name + " (" + originalURI.host +")";
> 
> This should be a localizable string.

I will add a localizable string in subscribe.properties.

> 
> @@ +659,5 @@
> > +	  originalHandlerName = defaultHandler.name;
> > +	}
> > +        let perms = [originalHandlerName,
> > +	             this.request.title,
> > +                     this.request.uri.host,
> 
> This can throw.
> 
> More generally, I think this should show the hostname from the principal
> instead of from the URI (so that if evil.com opens a data: uri in a new
> window that requests this, we show evil.com and not nothing). That might
> need changing in more places, if you copied this from elsewhere, in which
> case please file a bug on this.
> 
> @@ +660,5 @@
> > +	}
> > +        let perms = [originalHandlerName,
> > +	             this.request.title,
> > +                     this.request.uri.host,
> > +                     this.request.type];
> 
> Nit: more typical style is:
> 
> let perms = [
>   ...,
>   ...,
> ];

I am sorry that I post the wrong patch which is before I checking space/tab and empty lines.
I will doing followings in next version

1. Modify the patch according to your above suggestions
   using principal and uri in correct way.
2. add testcases for this patch
3. invoke Paolo to review permission ui part.
4. using mozreview.
5. make sure I attach the correct patch which checking space/tab and empty lines.

About what I want to discuss in Hawaii is that let you understand the code architecture and logic about this patch, and make sure the architecture and logic is correct before the pure code review. :)

Comment 57

[:Gijs (he/him)]

(In reply to Eden Chuang[:edenchuang] from comment #56)
> (In reply to :Gijs Kruitbosch from comment #54)
> > It's pretty odd that we're getting the handler info from one service and
> > storing it in another. Is there no fetching API on nsIHandlerService, or
> > storing API on nsIExtProtSvc?
> > 
> 
> nsIExternalProtocolService::getProtocolHandlerInfo will try to get protocol
> handler from OS setting first, then if it can get the protocol handler
> information from our database, mimeTypes.rdf, through nsIHandlerService, and
> it drops the information from OS. 
> 
> https://dxr.mozilla.org/mozilla-central/source/uriloader/exthandler/
> nsExternalHelperAppService.cpp#1155

Why does the privileging of the OS defaults matter in this case? We're overriding those defaults in this code, and all we really need is a ref to a handlerInfo object - the current values aren't relevant. I think my main point is that we have 2 services for 1 purpose, which is dumb. We can fix it in a separate bug.

> In fact, I just reuse the code from original handler registration in
> WebContentConverter.js 
> 
> https://dxr.mozilla.org/mozilla-central/source/browser/components/feeds/
> WebContentConverter.js#440

Right, so, this is why using mozreview would be helpful.

> > So this is generally odd... can't foo.com add a handler for mail.foo.com,
> > thus causing the permission to be on foo.com but the handler to be for
> > mail.foo.com? That's what I would expect. The current code stores the
> > permission for this._principal, not this._uri which is used for the
> > handler's URI template - so they could be different.
> > 
> 
> might be we can use testExactPermission() and addPermission() to replace
> testExactPermissionFromPrincipal() and addPermissionFromPrincipal(). But in
> fact testExactPermission() and addPermission() are a wrapper function of
> testExactPermissionPrincipal and addPermissionFromPrincipal(), I am not sure
> is this you want?

No. The point is that the URI that requests to save a protocol handler and the URI of the protocol handler don't necessarily share or subsume each other's principals, so we have to use the right one to store and check these permissions, we can't just pick whichever we like. I think you should be using this._principal instead of this._uri.

> > It seems to me that it would make more sense to enumerate all the URIs that
> > have a permission stored for the protocol using the permissions service, and
> > revoke any permissions that aren't for the URI we're currently dealing with.
> > That avoids the uri vs. uriTemplate ambiguity, as well as ensuring we revoke
> > permissions for sites where the user has manually removed the handler using
> > the preferences.
> 
> Yes, just like you mentioned, we can traverse all permissions for the type
> through the permissions service, and revoke the uri that aren't we are
> dealing with. 
> 
> But here is problem that we pre-load some protocol handlers, such as mibbit
> for irc, gmail, yahoo mail for mailto, and we don't give a permission when
> we pre-load it, we can't get the handler's permission from permissions
> service. To resolve this problem, I traverse the all possible handlers and
> try to create a permission for them.

There should be comments in the code for "gotcha"s like this. But besides that, it feels like we should be ensuring that the permissions match the default protocol handlers, so maybe we should be storing those permissions explicitly in the default permissions store. That would also fix this problem, make the code more intuitive, and make the permissions store correspond to the default protocol handlers.

> > @@ +294,5 @@
> > > +                            QueryInterface(Ci.nsIWebHandlerApp);
> > > +      if (modifiedHandler) {
> > > +	let modifiedURI = Services.io.newURI(modifiedHandler.uriTemplate, null, null);
> > > +        let modifiedPrincipal = Services.scriptSecurityManager.
> > > +	    createCodebasePrincipal(modifiedURI, {});
> > 
> > Again, not convinced how this works with private browsing and containers
> 
> In fact, private browsing is not allow to register any content handler.
> Please see
> 
> https://dxr.mozilla.org/mozilla-central/source/browser/components/feeds/
> WebContentConverter.js#413
> 
> if user is in pb, the API will do nothing and return out directly.

OK, but the origin attributes stuff still stands.

> > Pretty sure we should be reusing a principal from somewhere that invoked
> > this code rather than making it up as we go along. This won't have the right
> > originAttributes, private browsing flags, etc.
> > 
> 
> PermissionUI needs a principal to get the permission status, when prompt()
> is called. 
> 
> https://dxr.mozilla.org/mozilla-central/source/browser/modules/PermissionUI.
> jsm#275
> 
> Of course, we can use uri to replace principal by using testExactPerimssion()

If there are 2 APIs and one takes a principal and the other a URI, and you have a principal, you should use the principal API. If you don't have a principal, you should use the URI API and trust that it does the Right Thing.

Here, you're doing the opposite and creating the principal yourself, which is wrong.

> https://dxr.mozilla.org/mozilla-central/source/extensions/cookie/
> nsPermissionManager.cpp#164
> 
> I am not sure could we get a principal from somewhere, but I will try it.

Both in the e10s and the non-e10s case you at some point have a reference to the content window that requests the registration (which might be a frame, ie not a toplevel - the existing code doesn't currently deal with that well and that should be improved). You can use its document.nodePrincipal. In the e10s case, you will need to pass it along through the message manager.

> About what I want to discuss in Hawaii is that let you understand the code
> architecture and logic about this patch, and make sure the architecture and
> logic is correct before the pure code review. :)

Email me with some suggested times to meet?

Comment 58

[Eden Chuang[:edenchuang]]

Attachment: Bug 1270416 - registerProtocolHandler improvement with Permission UI and control center. Amadini

Review commit: https://reviewboard.mozilla.org/r/98620/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/98620/

Comment 59

[:Gijs (he/him)]

(sorry for the delay, I will try to get to this ASAP tomorrow morning.)

Comment 60

[:Gijs (he/him)]

Comment on attachment 8818598 [details]
Bug 1270416 - registerProtocolHandler improvement with Permission UI and control center.  Amadini

https://reviewboard.mozilla.org/r/98620/#review99390

::: browser/components/feeds/WebContentConverter.js:11
(Diff revision 1)
>  Components.utils.import("resource://gre/modules/Services.jsm");
>  Components.utils.import("resource://gre/modules/XPCOMUtils.jsm");
>  Components.utils.import("resource://gre/modules/PrivateBrowsingUtils.jsm");
>  
> +XPCOMUtils.defineLazyModuleGetter(this, "PermissionUI",
> +                                        "resource:///modules/PermissionUI.jsm");

Nit: indenting is off here. Either 2-space indent or align with `this`.

::: browser/components/feeds/WebContentConverter.js:280
(Diff revision 1)
> +    handlerInfo.preferredAction = Ci.nsIHandlerInfo.useHelperApp;
> +
> +    let hs = Cc["@mozilla.org/uriloader/handler-service;1"].
> +             getService(Ci.nsIHandlerService);
> +    hs.store(handlerInfo);
> +

Nit: remove this blank line

::: browser/components/feeds/WebContentConverter.js:284
(Diff revision 1)
> +    hs.store(handlerInfo);
> +
> +  },
> +
> +  _setupPermission(aAction) {
> +

And this one

::: browser/components/feeds/WebContentConverter.js:285
(Diff revision 1)
> +
> +  },
> +
> +  _setupPermission(aAction) {
> +
> +    let permsSvc = Services.perms;

Just use Services.perms throughout.

::: browser/components/feeds/WebContentConverter.js:286
(Diff revision 1)
> +    if (permsSvc.testExactPermission(this._uri, this.permissionKey)
> +        != permsSvc.UNKNOWN_ACTION) {
> +      permsSvc.remove(this._uri, this.permissionKey);
> +    }

Does the permissions service not deal with this? I would have assumed that the permission would just get replaced with the new one if you called .add() and an existing permission existed. That's certainly what the documentation says: http://searchfox.org/mozilla-central/source/netwerk/base/nsIPermissionManager.idl#65-69

::: browser/components/feeds/WebContentConverter.js:287
(Diff revision 1)
> +
> +  _setupPermission(aAction) {
> +
> +    let permsSvc = Services.perms;
> +    if (permsSvc.testExactPermission(this._uri, this.permissionKey)
> +        != permsSvc.UNKNOWN_ACTION) {

Do not split comparisons across lines like this. If necessary, save the result of testExactPermission to a temporary variable and then compare that in the if statement.

::: browser/components/feeds/WebContentConverter.js:300
(Diff revision 1)
> +    let permsEnum = permsSvc.enumerator;
> +    while ( permsEnum.hasMoreElements() ) {
> +      let perm = permsEnum.getNext().QueryInterface(Ci.nsIPermission);
> +      if (perm.type == this.permissionKey &&
> +          perm.capability != permsSvc.DENY_ACTION &&
> +	  !perm.matchesURI(this._uri, true)) {

Check the principal instead.

::: browser/components/feeds/WebContentConverter.js:302
(Diff revision 1)
> +        permsSvc.removePermission(perm);
> +        permsSvc.addFromPrincipal(modifiedPrincipal,
> +                                  this.permissionKey,
> +                                  permsSvc.DENY_ACTION,
> +                                  permsSvc.EXPIRE_NEVER);

Again, you don't need to call removePermission first.

::: browser/components/feeds/WebContentConverter.js:515
(Diff revision 1)
>      Utils.checkProtocolHandlerAllowed(aProtocol, aURIString,
>                                        haveWindow ? aBrowserOrWindow : null);

Please file a bug on, and make sure we fix reasonably soon, making these checks more meaningful.

::: browser/components/feeds/WebContentConverter.js:518
(Diff revision 1)
> -    // Now Ask the user and provide the proper callback
> -    let message = this._getFormattedString("addProtocolHandler",
> -                                           [aTitle, uri.host, aProtocol]);
> -
> -    let notificationIcon = uri.prePath + "/favicon.ico";
> +    let handlerRegistrationRequest =
> +        new HandlerRegistrationRequest(browser,
> +                                       uri,
> +                                       aTitle,
> +                                       aProtocol);

So instead, make the constructor take a principal, and use the principal of the contentWindow. If we don't have a contentWindow here (ie in the e10s case), ensure we pass it in as a parameter in the message we send from the content process.

::: browser/components/feeds/test/browser/browser.ini:2
(Diff revision 1)
> +[DEFAULT]
> +head = head.js

This isn't an xpcshell manifest, so you should list head.js under support-files.

::: browser/components/feeds/test/browser/browser_bug1270416.js:1
(Diff revision 1)
> +"use strict";

please name the test file something descriptive instead of just a bug number

::: browser/components/feeds/test/browser/browser_bug1270416.js:7
(Diff revision 1)
> +
> +/**
> + *  Test the notification actions' label and access key.
> + */
> +add_task(function*() {
> +

No empty lines at the beginning or end of a function.

::: browser/components/feeds/test/browser/browser_bug1270416.js:8
(Diff revision 1)
> +/**
> + *  Test the notification actions' label and access key.
> + */
> +add_task(function*() {
> +
> +  let notification = yield* openTestURL();

Each task should be opening (and at the end, closing) a tab, instead of manipulating the current tab.

::: browser/components/feeds/test/browser/head.js:1
(Diff revision 1)
> +Cu.import('resource://gre/modules/Services.jsm');

Basically, I think pretty much everything in this file should be in the test file, except maybe getPopupNotificationNode, clickAlwaysAllow and clickNeverAllow (which don't require any other global variables to work). The other helpers can be helpers, but they should live in the test.

::: browser/components/feeds/test/browser/head.js:3
(Diff revision 1)
> +Cu.import('resource://gre/modules/Services.jsm');
> +
> +var gPermsSvc = Services.perms;

Use `let` or `const` (throughout this file).

::: browser/components/feeds/test/browser/head.js:10
(Diff revision 1)
> +                     getService(Ci.nsIHandlerService);
> +var gExtProtocolSvc = Cc["@mozilla.org/uriloader/external-protocol-service;1"].
> +                         getService(Ci.nsIExternalProtocolService);
> +
> +var testURL = "http://example.com/browser/" +
> +  "browser/components/feeds/test/browser/protocolHandler.html";

Don't hardcode this, use something like this instead:

https://dxr.mozilla.org/mozilla-central/source/toolkit/components/reader/test/browser_readerMode.js#13

But also, this should be in the test file, not in head.js . If you think this is going to be useful for more test, use a more descriptive variable name with the g or k prefix.

::: browser/components/feeds/test/browser/head.js:13
(Diff revision 1)
> +
> +var testPermissionKey = "handler-registration-testprotocol";
> +var testURI = Services.io.newURI("https://example.com/foobar?uri=%s", null, null);
> +var defaultURI = Services.io.newURI("http://test.org/testProtocol=%s", null, null);
> +var defaultHandlerName = "Test Protocol default"
> +var browser = gBrowser.selectedBrowser;

These things do not belong in head.js

::: browser/components/feeds/test/browser/head.js:16
(Diff revision 1)
> +var kTestProtocolType = "testprotocol";
> +var kTestNotificationID = "handler-registrations-testprotocol-notificationID";
> +
> +var testPermissionKey = "handler-registration-testprotocol";
> +var testURI = Services.io.newURI("https://example.com/foobar?uri=%s", null, null);
> +var defaultURI = Services.io.newURI("http://test.org/testProtocol=%s", null, null);

We don't map test.org ( https://dxr.mozilla.org/mozilla-central/source/testing/mozbase/mozprofile/tests/server_locations.py ) so this is going to cause failures if we ever try following the link. Use one of the other hosts.

::: browser/components/feeds/test/browser/head.js:82
(Diff revision 1)
> +  browser.loadURI(testURL);
> +  yield BrowserTestUtils.browserLoaded(browser, testURL);

Use BrowserTestUtils.openNewForegroundTab() instead.

::: browser/components/feeds/test/browser/protocolHandler.html:1
(Diff revision 1)
> +<!DOCTYPE html>

This is copied from the file in browser/base/content/test/general/.

Please use "hg mv" instead, both for this and the browser test itself (and remove it from the corresponding ini file), because right now the test in browser/base/content/test/general/ will continue failing because we changed the UI for this API.

::: browser/components/feeds/test/browser/protocolHandler.html:14
(Diff revision 1)
> +    <script type="text/javascript">
> +      navigator.registerProtocolHandler("testprotocol",
> +          "https://example.com/foobar?uri=%s",
> +          "Test Protocol");
> +    </script>
> +    <a id="link" href="testprotocol:test">testprotocol link</a>

We should test that after registering the protocol, it actually works.

::: browser/components/feeds/test/mochitest.ini:15
(Diff revision 1)
>  
>  [test_bug436801.html]
>  [test_bug494328.html]
>  [test_bug589543.html]
>  [test_registerHandler.html]
> +#[test_registerHandler2.html]

Why is this commented out?

::: browser/locales/en-US/chrome/browser/feeds/subscribe.properties:50
(Diff revision 1)
>  feedSubscriptionAudioPodcast2=You can subscribe to this podcast to receive updates when this content changes.
>  feedSubscriptionVideoPodcast2=You can subscribe to this video podcast to receive updates when this content changes.
>  
>  # Protocol Handling
> -# "Add %appName (%appDomain) as an application for %protocolType links?"
> -addProtocolHandler=Add %S (%S) as an application for %S links?
> +# "Would you like to change %originalDefaultAppName to %appName (%appDomain) to be the default application for %protocolType links?"
> +replaceDefaultProtocolHandler=Would you like to change %S to %S (%S) to be the default application for %S links?

More correct English would be:

Would you like %S (%S) to replace %S as the default application for %S links?

Or even:

Would you like %S (%S) to handle %S links, replacing %S?

::: browser/locales/en-US/chrome/browser/feeds/subscribe.properties:54
(Diff revision 1)
> -# "Add %appName (%appDomain) as an application for %protocolType links?"
> -addProtocolHandler=Add %S (%S) as an application for %S links?
> -addProtocolHandlerAddButton=Add Application
> -addProtocolHandlerAddButtonAccesskey=A
> +# "Would you like to change %originalDefaultAppName to %appName (%appDomain) to be the default application for %protocolType links?"
> +replaceDefaultProtocolHandler=Would you like to change %S to %S (%S) to be the default application for %S links?
> +# "Set %appName (%appDomain) as the default application for %protocolType links?"
> +setDefaultProtocolHandler=Set %S (%S) as the default application for %S links?
> +# "%originalDefaultAppName (%originalDefaultAppDomain)"
> +originalDefaultProtocolHandler=%S (%S)

Nested replaces are frowned upon, because in some language declensions etc. will have to be changed. Please just create a copy of the replaceDefaultProtocolHandler string that has an extra replacement param.

::: browser/modules/PermissionUI.jsm:81
(Diff revision 1)
> +function LOG(aMsg) {
> +  dump("@ PermissionUI: " + aMsg + "\n");
> +}

This is unused, please remove it.

::: browser/modules/PermissionUI.jsm:120
(Diff revision 1)
>    /**
> +   * Returns the nsIURI associated with the request.
> +   *
> +   * Subclasses must override this.
> +   *
> +   * @return {nsIURI}
> +   */
> +  get uri() {
> +    throw new Error("Not implemented.");
> +  },
> +
> +  /**

I'm really confused. I thought in Hawaii we said we could just use the principal's URI everywhere? We can implement a URI getter that returns the principal's URI, I guess, but then we shouldn't implement a no-implement handler and force all the subclasses to override it.

If there is some reason that doesn't work, then it feels like we shouldn't clutter up the generic implementation of PermissionsUI with this field if only our notification needs it to be different from the principal's URI.

::: browser/modules/PermissionUI.jsm:296
(Diff revision 1)
> -      let result =
> +      let result;
> +      if (this.principal) {
> +        result =
> -        Services.perms.testExactPermissionFromPrincipal(this.principal,
> +          Services.perms.testExactPermissionFromPrincipal(this.principal,
> -                                                        this.permissionKey);
> +                                                          this.permissionKey);

We should always only use the principal here.

Comment 61

[:Gijs (he/him)]

We really need to make sure that we add some checks to this stuff soon. Also, please test what happens when using a very long protocol name or handling URI. We should make sure the UI stays usable (so malicious webpages can't abuse the UI to make the browser unusable for users). I also think we should have a list of allowed protocols instead of a list of blocked protocols like we do now.

Comment 62

[:Paolo Amadini]

(In reply to :Gijs Kruitbosch from comment #61)
> I also think we should have a list of allowed protocols instead of a list of blocked
> protocols like we do now.

I think the permissionKey should be just one for all protocols, to prevent spamming by using an infinite number of protocol names. If there is a whitelist instead of a blacklist, this concern is slightly alleviated.

Regardless of whether there are one or many permissions, we must make sure that they appear in the Control Center, so the decision can be reverted if the permission has been denied previously. If there are multiple permissions, this means we need to add custom code to either show the protocol names for each line, or show all permissions on one line with the "X" reverting them all.

Comment 63

[Eden Chuang[:edenchuang]]

Comment on attachment 8818598 [details]
Bug 1270416 - registerProtocolHandler improvement with Permission UI and control center.  Amadini

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/98620/diff/1-2/

Comment 64

[Eden Chuang[:edenchuang]]

> 
> ::: browser/components/feeds/WebContentConverter.js:515
> (Diff revision 1)
> >      Utils.checkProtocolHandlerAllowed(aProtocol, aURIString,
> >                                        haveWindow ? aBrowserOrWindow : null);
> 
> Please file a bug on, and make sure we fix reasonably soon, making these
> checks more meaningful.
> 

https://bugzilla.mozilla.org/show_bug.cgi?id=1324720

>
> ::: browser/components/feeds/test/browser/protocolHandler.html:1
> (Diff revision 1)
> > +<!DOCTYPE html>
> 
> This is copied from the file in browser/base/content/test/general/.
> 
> Please use "hg mv" instead, both for this and the browser test itself (and
> remove it from the corresponding ini file), because right now the test in
> browser/base/content/test/general/ will continue failing because we changed
> the UI for this API.
> 

I fixed the testing under browser/base/content/test/general/ 

>
> ::: browser/components/feeds/test/browser/protocolHandler.html:14
> (Diff revision 1)
> > +    <script type="text/javascript">
> > +      navigator.registerProtocolHandler("testprotocol",
> > +          "https://example.com/foobar?uri=%s",
> > +          "Test Protocol");
> > +    </script>
> > +    <a id="link" href="testprotocol:test">testprotocol link</a>
>
> We should test that after registering the protocol, it actually works.

We have already a test uriloader/exthandler/tests/mochitest/browser_web_protocol_handlers.js for testing a testprotocol link with the registered handler. I reuse it to test the same issue.
In addition to the test link, I also create a test for super long protocol issue mentioned in comment 61.

Comment 65

[Eden Chuang[:edenchuang]]

(In reply to :Paolo Amadini from comment #62)
> (In reply to :Gijs Kruitbosch from comment #61)
> > I also think we should have a list of allowed protocols instead of a list of blocked
> > protocols like we do now.
> 
> I think the permissionKey should be just one for all protocols, to prevent
> spamming by using an infinite number of protocol names. If there is a
> whitelist instead of a blacklist, this concern is slightly alleviated.
> 
> Regardless of whether there are one or many permissions, we must make sure
> that they appear in the Control Center, so the decision can be reverted if
> the permission has been denied previously. If there are multiple
> permissions, this means we need to add custom code to either show the
> protocol names for each line, or show all permissions on one line with the
> "X" reverting them all.

Yes, we should integrate the permission control in control center, it will be the second part. Currently I focus on the first part "registerProtocolHandler with PermissionUI". Of course, they should be landed together.

Comment 66

[:Gijs (he/him)]

Comment on attachment 8818598 [details]
Bug 1270416 - registerProtocolHandler improvement with Permission UI and control center.  Amadini

https://reviewboard.mozilla.org/r/98620/#review100234

I think moving the test is the right thing to do. You haven't really said why you haven't - could you please elaborate on why you ignored that part of my review feedback?

Please also move the uriloader test that you're modifying in the same way.

What does the permission popup actually look like with the very long protocol at this stage - can you provide a screenshot and get UX review?

Finally, please do a trypush for your next version and ensure that all the tests pass.

::: browser/components/feeds/WebContentConverter.js:254
(Diff revision 2)
> +  get uri() {
> +    return this._handlerURI;
> +  },

This is unused, it looks like.

::: browser/components/feeds/WebContentConverter.js:294
(Diff revision 2)
> +    while ( permsEnum.hasMoreElements() ) {
> +      let perm = permsEnum.getNext().QueryInterface(Ci.nsIPermission);
> +      if (perm.type == this.permissionKey &&
> +          perm.capability != Services.perms.DENY_ACTION &&
> +	  !perm.principal.equals(this._principal)) {
> +        LOG("Set " + perm.type + " permission of " + perm.principal.URI.spec + " as DENY_ACTION");

Is it really useful to log this?

::: browser/components/feeds/test/browser/browser_registerHandlerWithPermissionUI.js:10
(Diff revision 2)
> +let kTestRoot = getRootDirectory(gTestPath).replace("chrome://mochitests/content",
> +                                                      "http://example.com");

Indentation is wrong here.

::: browser/components/feeds/test/browser/browser_registerHandlerWithPermissionUI.js:12
(Diff revision 2)
> +let kTestProtocolType = "testprotocol";
> +let kTestNotificationID = "handler-registrations-testprotocol-notificationID";
> +let kTestPermissionKey = "handler-registration-testprotocol";

Use const instead of let

::: browser/components/feeds/test/browser/browser_registerHandlerWithPermissionUI.js:24
(Diff revision 2)
> +  return gExtProtocolSvc.getProtocolHandlerInfo(aProtocolType);
> +}
> +
> +function setupDefaultHandler()
> +{
> +  let handler = Cc["@mozilla.org/uriloader/web-handler-app;1"].

You only call this function once, so this should probably just live in the global scope, perhaps as a self-executing function expression so you don't pollute the global scope.

::: browser/components/feeds/test/browser/browser_registerHandlerWithPermissionUI.js:202
(Diff revision 2)
> +    gPermsSvc.removeFromPrincipal(browser.contentPrincipal, kTestPermissionKey);
> +    gHandlerSvc.remove(handlerInfo);

You should re-set the 'deny' permission for the default handler here in order to check it correctly in the other tests in this file.

::: browser/components/feeds/test/browser/browser_registerHandlerWithPermissionUI.js:210
(Diff revision 2)
> +let kSuperLongTestProtocolType =
> +  "veryveryveryveryveryveryveryveryveryveryveryveryveryveryveryveryveryveryveryveryveryverylongprotocol";
> +let kSuperLongTestNotificationID =
> +  "handler-registrations-veryveryveryveryveryveryveryveryveryveryveryveryveryveryveryveryveryveryveryveryveryverylongprotocol-notificationID";
> +let kSuperLongTestPermissionKey =
> +  "handler-registration-veryveryveryveryveryveryveryveryveryveryveryveryveryveryveryveryveryveryveryveryveryverylongprotocol";

You're using the 'k' prefix for constants, but not declaring these with `const`.

You should reuse `kSuperLongTestProtocolType` in defining the other two strings.

::: browser/components/feeds/test/browser/browser_registerHandlerWithPermissionUI.js:217
(Diff revision 2)
> +add_task(function*() {
> +  yield BrowserTestUtils.withNewTab({

Instead of having 2 separate tasks, please just loop over the two files and protocol handlers and run the same test(s) with both of them.

::: browser/components/feeds/test/browser/browser_registerHandlerWithPermissionUI.js:255
(Diff revision 2)
> +    yield BrowserTestUtils.waitForCondition(() => gURLBar.value != kTestRoot + "superLongProtocolHandler.html");
> +    is(gURLBar.value, expectedURL,
> +       "the expected URL is displayed in the location bar");

Why is this a separate waitForCondition? We shouldn't add more uses of waitForCondition. Can we wait for this some other way, e.g. by using browserStopped(browser) ?

::: browser/components/feeds/test/browser/head.js:7
(Diff revision 2)
> +function clickAlwaysAllow() {
> +  let removePromise =
> +    BrowserTestUtils.waitForEvent(PopupNotifications.panel, "popuphidden");
> +  let popupNotification = getPopupNotificationNode();
> +  popupNotification.button.click();
> +  return removePromise;
> +}
> +
> +function clickNeverAllow() {
> +  let removePromise =
> +    BrowserTestUtils.waitForEvent(PopupNotifications.panel, "popuphidden");
> +  let popupNotification = getPopupNotificationNode();
> +  popupNotification.secondaryButton.click();
> +  return removePromise;
> +}

These two functions are the same. Factor out the bits that are the same, please.

Comment 67

[Francesco Lodolo [:flod]]

Comment on attachment 8818598 [details]
Bug 1270416 - registerProtocolHandler improvement with Permission UI and control center.  Amadini

https://reviewboard.mozilla.org/r/98620/#review100256

::: browser/locales/en-US/chrome/browser/feeds/subscribe.properties:57
(Diff revision 2)
> -addProtocolHandlerAddButtonAccesskey=A
> +# for the case that default handler is a web app.
> +# "Would you like %originalDefaultAppName (%originalDefaultAppDomain) to replace %appName (%appDomain) as the default application for %protocolType links?"
> +replaceDefaultWebProtocolHandler=Would you like %S (%S) to replace %S (%S) as the default application for %S links?
> +# "Set %appName (%appDomain) as the default application for %protocolType links?"
> +setDefaultProtocolHandler=Set %S (%S) as the default application for %S links?
> +handlerRegistration.alwaysAllow=Always Allow

Could we have these called X.label and X.accesskey, to make tools' life easier (when they try to match label to accesskey)?

Comment 68

[Eden Chuang[:edenchuang]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b18d94062c3f

Comment 69

[Eden Chuang[:edenchuang]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a7df55eec591

Comment 70

[Eden Chuang[:edenchuang]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=70cb20c9e25c

Comment 71

[Eden Chuang[:edenchuang]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3a5aa914ea88

Comment 72

[Eden Chuang[:edenchuang]]

Comment on attachment 8818598 [details]
Bug 1270416 - registerProtocolHandler improvement with Permission UI and control center.  Amadini

https://reviewboard.mozilla.org/r/98620/#review100234

> This is unused, it looks like.

No, it will be used in PermissionUI.jsm for prompt message

> You only call this function once, so this should probably just live in the global scope, perhaps as a self-executing function expression so you don't pollute the global scope.

Because the function will be called multiple times, I kill it as original.

> You should re-set the 'deny' permission for the default handler here in order to check it correctly in the other tests in this file.

The default handler's permission will be cleanup while test finishes. setupDefaultHandler() will register a cleanup function while it is called. So I didn't clean it at the end of test.

> Why is this a separate waitForCondition? We shouldn't add more uses of waitForCondition. Can we wait for this some other way, e.g. by using browserStopped(browser) ?

I tried browserStopped and waitForLocationChange. But it doesn't work. Both they make test failed on try(but passed on my local), unexpected new window and new tab will be detected while test finsihed, even I called closeWindow and removeTab. I have no idea why they doesn't work.

Comment 73

[Eden Chuang[:edenchuang]]

Comment on attachment 8818598 [details]
Bug 1270416 - registerProtocolHandler improvement with Permission UI and control center.  Amadini

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/98620/diff/2-3/

Comment 74

[Eden Chuang[:edenchuang]]

Comment on attachment 8818598 [details]
Bug 1270416 - registerProtocolHandler improvement with Permission UI and control center.  Amadini

https://reviewboard.mozilla.org/r/98620/#review100234

I move/merge these tests in the new patch. I didn't do that is because I am not sure are these tests belong to this test scope.
For example, I think browser_privateBrowsing_protocolHandler.js should belong to the privatebrowsing scope, such that privatebrowsing developers needn't to take care other scopes problem during developing.

The try run result is here
https://treeherder.mozilla.org/#/jobs?repo=try&revision=653518f100220f4601f3bfc6e3f2097afe62fbdc&selectedJob=33298113

I will provide the screenshot later.

Comment 75

[:Gijs (he/him)]

Comment on attachment 8818598 [details]
Bug 1270416 - registerProtocolHandler improvement with Permission UI and control center.  Amadini

https://reviewboard.mozilla.org/r/98620/#review101020

::: browser/components/feeds/test/browser/browser_registerHandlerWithPermissionUI.js:17
(Diff revisions 2 - 3)
> +const kTestObjects = [
> +  {
> +    tabInfo: {
> +      gBrowser,
> +      url: kTestRoot + "protocolHandler.html",
> +    },
> +    protocolType: kTestProtocol,
> +    permissionKey: "handler-registration-" + kTestProtocol,
> +    notificationID: "handler-registrations-" + kTestProtocol + "-notificationID",
> +    handlerName: "Test Protocol",
> +    handlerUriTemplate: "https://example.com/foobar?uri=%s",
> +    expectedURL: "https://example.com/foobar?uri=" + kTestProtocol + "%3Atest",
> +    needDefaultHandler: false,

The second test object is basically the same as the first, but with needDefaultHandler turned on.

Can we dedupe this and also test the needDefaultHandler off case for the longer protocol?

::: browser/components/feeds/test/browser/browser_registerHandlerWithPermissionUI.js:19
(Diff revisions 2 - 3)
> +    tabInfo: {
> +      gBrowser,
> +      url: kTestRoot + "protocolHandler.html",
> +    },

You can just pass only the URL, and repeating `kTestRoot` in the test object list seems superfluous as well.

::: browser/components/feeds/test/browser/browser_registerHandlerWithPermissionUI.js:24
(Diff revisions 2 - 3)
> +    permissionKey: "handler-registration-" + kTestProtocol,
> +    notificationID: "handler-registrations-" + kTestProtocol + "-notificationID",

Hm, I hadn't noticed this before, but can we make these consistent? Probably "handler-registration", without the 's' at the end.

::: browser/components/feeds/test/browser/browser_registerHandlerWithPermissionUI.js:37
(Diff revisions 2 - 3)
> +    permissionKey: "handler-registration-" + kTestProtocol,
> +    notificationID: "handler-registrations-" + kTestProtocol + "-notificationID",
> +    handlerName: "Test Protocol",
> +    handlerUriTemplate: "https://example.com/foobar?uri=%s",
> +    expectedURL: "https://example.com/foobar?uri=" + kTestProtocol + "%3Atest",

All of these, except the handlerName, are the same between tests besides the protocol name. Can you use template strings in the test loop instead (inserting the `protocolType` string)?

::: browser/components/feeds/test/browser/browser_registerHandlerWithPermissionUI.js:111
(Diff revisions 2 - 3)
> +  let tab = yield promiseTabOpened;
> +  gBrowser.selectedTab = tab;
> +  is(gURLBar.value, expectedURL,
> +     "the expected URL is displayed in the location bar");

Does this work in e10s? Assigning to `selectedTab` isn't normally synchronous. I'd expect you would have to use `BTU.switchTab` instead. Or you can also pass `shiftKey` is true and just ensure the tab is opened in the foreground...

::: browser/components/feeds/test/browser/browser_registerHandlerWithPermissionUI.js:123
(Diff revisions 2 - 3)
> +  let newWindowPromise = BrowserTestUtils.waitForNewWindow();
> +  yield BrowserTestUtils.synthesizeMouseAtCenter(link, {shiftKey: true},
> +                                                 aBrowser);
> +  let win = yield newWindowPromise;
> +  yield BrowserTestUtils.browserLoaded(win.gBrowser.selectedBrowser);
> +  yield BrowserTestUtils.waitForCondition(() => win.gBrowser.currentURI.spec == expectedURL);

Again, we should not add new `waitForCondition` calls.

I don't think testing the new window test is adding value for the code that we're trying to test here. Just the new tab test and current tab test should be enough. Note that your trypushed code does:

```js
yield BTU.browserLoaded(browser);
yield BTU.browserStopped(browser);
```

which is likely to fail because the browser has already stopped (and the events browserStopped waits for have already happened) by the time you construct that promise. If using browserStopped, don't also use browserLoaded, and make sure you construct the promise before navigating, and wait for it after navigating.

::: browser/components/feeds/test/browser/browser_registerHandlerWithPermissionUI.js:144
(Diff revisions 2 - 3)
> -  }, function* (browser) {
> +    function* (browser) {
>      let notification =
> -      PopupNotifications.getNotification(kTestNotificationID, browser);
> +      PopupNotifications.getNotification(testObj.notificationID, browser);

The indenting here looks wrong, and I would expect we need to wait for the notification to appear after the tab has loaded on OSes where popups are animated.

::: browser/components/feeds/test/browser/browser_registerHandlerWithPermissionUI.js:170
(Diff revisions 2 - 3)
>  add_task(function*() {
> -  yield BrowserTestUtils.withNewTab({
> -    gBrowser,
> -    url: kTestRoot + "protocolHandler.html",
> -  }, function* (browser) {
> +  let testObj = kTestObjects[0];
> +  yield BrowserTestUtils.withNewTab(
> +    testObj.tabInfo,
> +    function* (browser) {
>       // check the current permission before clicking Never Allow
>      let curPerm = gPermsSvc.testExactPermissionFromPrincipal(browser.contentPrincipal,
> -                                                             kTestPermissionKey);
> +                                                             testObj.permissionKey);

Again, the indenting looks wrong here.

Why is this hardcoded to run against kTestObjects[0]? Why isn't this testing part of the loop below?

::: browser/components/feeds/test/browser/browser_registerHandlerWithPermissionUI.js:212
(Diff revisions 2 - 3)
> -    is(curPerm, gPermsSvc.UNKNOWN_ACTION,
> +      is(curPerm, gPermsSvc.UNKNOWN_ACTION,
> -       "Should be no permission set to begin with.");
> +         "Should be no permission.");

Please include the permission key in the test message so that the output is unique and test failures are easy to diagnose.

::: browser/components/feeds/test/browser/browser_registerHandlerWithPermissionUI.js:215
(Diff revisions 2 - 3)
> -/**
> - *  test the Always Allow action with a default handler.
> - *
> - *  if the Always Allow clicked, the permission should be set as ALLOW_ACTION,
> +      // setup a default handler if needed
> +      if (testObj.needDefaultHandler) {
> +        setupDefaultHandler(testObj.protocolType);
> +      }

My point that you ignored because of the registerCleanupFunction was that you should remove this at the end of this loop, so that the default handler doesn't interfere with the next iteration of the loop (so the next subtest in the file). This will also make it easier to test with/without the default handler: you can just create a doubly-nested loop:

```js
for (let testObj of kTestObjects) {
  for (let useDefaultHandler of [false, true]) {
    // ... run tests
  }
}
```

::: browser/components/feeds/test/browser/browser_registerHandlerWithPermissionUI.js:240
(Diff revisions 2 - 3)
> -    gPermsSvc.removeFromPrincipal(browser.contentPrincipal, kTestPermissionKey);
> -    gHandlerSvc.remove(handlerInfo);
> -  });
> +      // Get permission object here for cleanup.
> +      // browser.contentPrincipal will be modified after checking protocol link
> +      // are handled by registered handler.

This is only because you're using 'http' to load the page and the protocol handler is for 'https'. Just use https throughout and this problem will go away, and declare a `const permissionPrincipal = browser.contentPrincipal` at the start of the loop to ensure we're using the same principal throughout anyway.

::: browser/components/privatebrowsing/test/browser/browser_privatebrowsing_protocolhandler.js:31
(Diff revision 3)
>          // Make sure the notification is correctly displayed with a remember control
>          ok(notification, "Notification box should be displaying outside of private browsing mode");
>        }
>  
>        promiseFinished.resolve();
>      }, 100); // remember control is added in a setTimeout(0) call

This is no longer true now, right? Can we fix the test properly? I expect you need to wait for `popupshown` somewhere, because there's no guarantee that the notification is present synchronously, and the 100ms timeout is hacky.

Comment 76

[Eden Chuang[:edenchuang]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=84f7feda5ba6

Comment 77

[Eden Chuang[:edenchuang]]

Comment on attachment 8818598 [details]
Bug 1270416 - registerProtocolHandler improvement with Permission UI and control center.  Amadini

https://reviewboard.mozilla.org/r/98620/#review101020

> This is no longer true now, right? Can we fix the test properly? I expect you need to wait for `popupshown` somewhere, because there's no guarantee that the notification is present synchronously, and the 100ms timeout is hacky.

For the non-privatebrowsing case, I think we can use BTU.waitForEvent("popupshown") to make sure the notification is popup. But for the privatebrowsing case, we have no event to wait to make sure the notification is not popup. So I will still use the old way for privatebrowsing checking.

Comment 78

[Eden Chuang[:edenchuang]]

Comment on attachment 8818598 [details]
Bug 1270416 - registerProtocolHandler improvement with Permission UI and control center.  Amadini

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/98620/diff/3-4/

Comment 79

[:Gijs (he/him)]

Comment on attachment 8818598 [details]
Bug 1270416 - registerProtocolHandler improvement with Permission UI and control center.  Amadini

https://reviewboard.mozilla.org/r/98620/#review101274

Some last changes below, but with that this seems OK to land if try is green.

::: browser/components/feeds/test/browser/browser_registerHandlerWithPermissionUI.js
(Diff revisions 3 - 4)
> -    gPermsSvc.remove(kFakeDefaultURI, permissionKey);
>      if (gHandlerSvc.exists(getHandlerInfo(aProtocolType))) {
>        gHandlerSvc.remove(getHandlerInfo(aProtocolType));
>      }

I would keep the permission removal in the cleanup function, and factor out the registerCleanupFunction call to only happen once (at (more or less) the top of the file is fine).

::: browser/components/feeds/test/browser/browser_registerHandlerWithPermissionUI.js:253
(Diff revisions 3 - 4)
> -      gPermsSvc.removeFromPrincipal(perm.principal, testObj.permissionKey);
> +          // cleanup
> +          if (useDefaultHandler) {
> +            gPermsSvc.remove(kFakeDefaultURI, testObj.permissionKey);
> +          }
> +          gPermsSvc.removeFromPrincipal(principal, testObj.permissionKey);

Shouldn't this also remove the protocol (and the default protocol) from the handler service?

Comment 80

[Eden Chuang[:edenchuang]]

Comment on attachment 8818598 [details]
Bug 1270416 - registerProtocolHandler improvement with Permission UI and control center.  Amadini

https://reviewboard.mozilla.org/r/98620/#review101274

Gijs, thanks for your help to review this patch. Since I am not very familiar with JS, thanks for your patient.

> I would keep the permission removal in the cleanup function, and factor out the registerCleanupFunction call to only happen once (at (more or less) the top of the file is fine).

I will register the cleanup function in the final patch.

> Shouldn't this also remove the protocol (and the default protocol) from the handler service?

I think we needn't, because the default handler's protocol type is the same with the registered handler one, and we clean it in the following two line.

Comment 81

[Eden Chuang[:edenchuang]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=02208a2265eb

Comment 82

[Eden Chuang[:edenchuang]]

Attachment: Bug1270416_Improve_registerProtocolHandler_API_with_PermissionUI. r=Gijs

Comment 83

[Eden Chuang[:edenchuang]]

[Feature/regressing bug #]: Bug 1270416
[User impact if declined]: No impact. 
[Describe test coverage new/current, TreeHerder]: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d12b56ed2ff5e2152ad419195f77ad3275d91fec&selectedJob=33497808
[Risks and why]: low risk. Just improve UI for reigsterProtocolHandler with PermissionUI and set the registered handler as the default handler once user click "Always Allow".
[String/UUID change made/needed]: Yes

Comment 84

[Carsten Book [:Tomcat]]

Hi, could you take a look at the patch , applying bug1270416_final.patch
patch bug1270416_final.patch is empty

is what i got when trying to apply

Comment 85

[:Paolo Amadini]

Comment on attachment 8822311 [details] [diff] [review]
Bug1270416_Improve_registerProtocolHandler_API_with_PermissionUI. r=Gijs

We shouldn't land this in mozilla-central now. You can still consider this bug completed for tracking purposes, but the feature is not ready to ship, and given that this is not controlled by an about:config preference, we cannot land this bug now.

This bug should land together with the Control Center modifications. I'm setting a temporary r- as a reminder, but this can be lifted once the second part is ready to land.

Eden, isn't this also what you said in comment 65?

If we landed this, we would regress the experience because users would have no way to revert the choice once they denied a permission. At present, I don't even see a bug on file for implementing the second part of the UI in the Control Center, and we'd likely lose track of this regression.

Also, I don't see a whitelist implemented, but still the permission and notification keys are different for each protocol scheme. What happens if the page requests many schemes at the same time? Would we show many different anchor icons in the URL bar? Would we stack multiple notifications together on the same icon? Would we enqueue the requests one after the other?

Comment 86

[:Paolo Amadini]

(In reply to :Gijs (gone until 3 jan) from comment #66)
> What does the permission popup actually look like with the very long
> protocol at this stage - can you provide a screenshot and get UX review?

Eden, have you taken this screenshot and got UX review offline? Or has this become unnecessary?

It would be good to have it on the bug for reference anyways.

Comment 87

[Eden Chuang[:edenchuang]]

Attachment: super_long protocol_screen_shot.png

Hello Bryant

The attached is the screen shot of super long protocol handler registration.

Reviewers ask to review UX design for the case when the registered handler URI is very long. Could you take a look on it, and then tell me what we should do for it. Thanks.

Comment 88

[Bryant Mao [:bryantmao]]

(In reply to :Gijs (gone until 3 jan) from comment #66)
> Comment on attachment 8818598 [details]
> Bug 1270416 - Improve registerProtocolHandler API with PermissionUI. 
> Kruitbosch
> 
> https://reviewboard.mozilla.org/r/98620/#review100234
> 
> I think moving the test is the right thing to do. You haven't really said
> why you haven't - could you please elaborate on why you ignored that part of
> my review feedback?
> 
> Please also move the uriloader test that you're modifying in the same way.
> 
> What does the permission popup actually look like with the very long
> protocol at this stage - can you provide a screenshot and get UX review?
> 
To review the UI, I need to clarify two questions first. First, how long the string can possibly be, and how frequent this case might happen?
> Finally, please do a trypush for your next version and ensure that all the
> tests pass.
> 
> ::: browser/components/feeds/WebContentConverter.js:254
> (Diff revision 2)
> > +  get uri() {
> > +    return this._handlerURI;
> > +  },
> 
> This is unused, it looks like.
> 
> ::: browser/components/feeds/WebContentConverter.js:294
> (Diff revision 2)
> > +    while ( permsEnum.hasMoreElements() ) {
> > +      let perm = permsEnum.getNext().QueryInterface(Ci.nsIPermission);
> > +      if (perm.type == this.permissionKey &&
> > +          perm.capability != Services.perms.DENY_ACTION &&
> > +	  !perm.principal.equals(this._principal)) {
> > +        LOG("Set " + perm.type + " permission of " + perm.principal.URI.spec + " as DENY_ACTION");
> 
> Is it really useful to log this?
> 
> :::
> browser/components/feeds/test/browser/
> browser_registerHandlerWithPermissionUI.js:10
> (Diff revision 2)
> > +let kTestRoot = getRootDirectory(gTestPath).replace("chrome://mochitests/content",
> > +                                                      "http://example.com");
> 
> Indentation is wrong here.
> 
> :::
> browser/components/feeds/test/browser/
> browser_registerHandlerWithPermissionUI.js:12
> (Diff revision 2)
> > +let kTestProtocolType = "testprotocol";
> > +let kTestNotificationID = "handler-registrations-testprotocol-notificationID";
> > +let kTestPermissionKey = "handler-registration-testprotocol";
> 
> Use const instead of let
> 
> :::
> browser/components/feeds/test/browser/
> browser_registerHandlerWithPermissionUI.js:24
> (Diff revision 2)
> > +  return gExtProtocolSvc.getProtocolHandlerInfo(aProtocolType);
> > +}
> > +
> > +function setupDefaultHandler()
> > +{
> > +  let handler = Cc["@mozilla.org/uriloader/web-handler-app;1"].
> 
> You only call this function once, so this should probably just live in the
> global scope, perhaps as a self-executing function expression so you don't
> pollute the global scope.
> 
> :::
> browser/components/feeds/test/browser/
> browser_registerHandlerWithPermissionUI.js:202
> (Diff revision 2)
> > +    gPermsSvc.removeFromPrincipal(browser.contentPrincipal, kTestPermissionKey);
> > +    gHandlerSvc.remove(handlerInfo);
> 
> You should re-set the 'deny' permission for the default handler here in
> order to check it correctly in the other tests in this file.
> 
> :::
> browser/components/feeds/test/browser/
> browser_registerHandlerWithPermissionUI.js:210
> (Diff revision 2)
> > +let kSuperLongTestProtocolType =
> > +  "veryveryveryveryveryveryveryveryveryveryveryveryveryveryveryveryveryveryveryveryveryverylongprotocol";
> > +let kSuperLongTestNotificationID =
> > +  "handler-registrations-veryveryveryveryveryveryveryveryveryveryveryveryveryveryveryveryveryveryveryveryveryverylongprotocol-notificationID";
> > +let kSuperLongTestPermissionKey =
> > +  "handler-registration-veryveryveryveryveryveryveryveryveryveryveryveryveryveryveryveryveryveryveryveryveryverylongprotocol";
> 
> You're using the 'k' prefix for constants, but not declaring these with
> `const`.
> 
> You should reuse `kSuperLongTestProtocolType` in defining the other two
> strings.
> 
> :::
> browser/components/feeds/test/browser/
> browser_registerHandlerWithPermissionUI.js:217
> (Diff revision 2)
> > +add_task(function*() {
> > +  yield BrowserTestUtils.withNewTab({
> 
> Instead of having 2 separate tasks, please just loop over the two files and
> protocol handlers and run the same test(s) with both of them.
> 
> :::
> browser/components/feeds/test/browser/
> browser_registerHandlerWithPermissionUI.js:255
> (Diff revision 2)
> > +    yield BrowserTestUtils.waitForCondition(() => gURLBar.value != kTestRoot + "superLongProtocolHandler.html");
> > +    is(gURLBar.value, expectedURL,
> > +       "the expected URL is displayed in the location bar");
> 
> Why is this a separate waitForCondition? We shouldn't add more uses of
> waitForCondition. Can we wait for this some other way, e.g. by using
> browserStopped(browser) ?
> 
> ::: browser/components/feeds/test/browser/head.js:7
> (Diff revision 2)
> > +function clickAlwaysAllow() {
> > +  let removePromise =
> > +    BrowserTestUtils.waitForEvent(PopupNotifications.panel, "popuphidden");
> > +  let popupNotification = getPopupNotificationNode();
> > +  popupNotification.button.click();
> > +  return removePromise;
> > +}
> > +
> > +function clickNeverAllow() {
> > +  let removePromise =
> > +    BrowserTestUtils.waitForEvent(PopupNotifications.panel, "popuphidden");
> > +  let popupNotification = getPopupNotificationNode();
> > +  popupNotification.secondaryButton.click();
> > +  return removePromise;
> > +}
> 
> These two functions are the same. Factor out the bits that are the same,
> please.

Comment 89

[Eden Chuang[:edenchuang]]

(In reply to :Paolo Amadini from comment #85)
> Comment on attachment 8822311 [details] [diff] [review]
> Bug1270416_Improve_registerProtocolHandler_API_with_PermissionUI. r=Gijs
> 
> We shouldn't land this in mozilla-central now. You can still consider this
> bug completed for tracking purposes, but the feature is not ready to ship,
> and given that this is not controlled by an about:config preference, we
> cannot land this bug now.
> 
> This bug should land together with the Control Center modifications. I'm
> setting a temporary r- as a reminder, but this can be lifted once the second
> part is ready to land.
> 
> Eden, isn't this also what you said in comment 65?
> 
> If we landed this, we would regress the experience because users would have
> no way to revert the choice once they denied a permission. At present, I
> don't even see a bug on file for implementing the second part of the UI in
> the Control Center, and we'd likely lose track of this regression.
> 
> Also, I don't see a whitelist implemented, but still the permission and
> notification keys are different for each protocol scheme. What happens if
> the page requests many schemes at the same time? Would we show many
> different anchor icons in the URL bar? Would we stack multiple notifications
> together on the same icon? Would we enqueue the requests one after the other?

Sorry, I might misunderstand the meaning of "seems OK to land" in comment 79.
I am working on the control center integration, after finish it I will send review to you.

Comment 90

[Eden Chuang[:edenchuang]]

Comment on attachment 8818598 [details]
Bug 1270416 - registerProtocolHandler improvement with Permission UI and control center.  Amadini

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/98620/diff/4-5/

Comment 91

[Eden Chuang[:edenchuang]]

Hello Gijs

Since the multiple registrations at the same time problem mentioned by Paolo in comment 85. I update the original patch for it. I sent review through moz-review, but the review flag seems be not modified by moz-review. So, should I modify the review flag by using bugzilla directly?

In the original patch, if the web site calls registerProtocolHandler multiple times, multiple popup notifications are shown to user together. In the updated patch, the notifications will popup one by one. The next popup notification is only shown after user clicks the "Always Allow" or "Never Allow" button.

Comment 92

[Masatoshi Kimura [:emk]]

(In reply to Eden Chuang[:edenchuang] from comment #91)
> Since the multiple registrations at the same time problem mentioned by Paolo
> in comment 85. I update the original patch for it. I sent review through
> moz-review, but the review flag seems be not modified by moz-review. So,
> should I modify the review flag by using bugzilla directly?

Yes, it is a known problem of MozReview (bug 1195661). The workaround is using bugzilla directly to modify the review flag.

Comment 93

[Eden Chuang[:edenchuang]]

try result of the review-requested patch

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8bbf4de965194f66a90dfd2942e3b4a54845a14a

Comment 94

[:Gijs (he/him)]

(In reply to Bryant Mao [:bryantmao] from comment #88)
> (In reply to :Gijs (gone until 3 jan) from comment #66)
> > What does the permission popup actually look like with the very long
> > protocol at this stage - can you provide a screenshot and get UX review?
> > 
> To review the UI, I need to clarify two questions first. First, how long the
> string can possibly be,

However long the website wants it to be. Could be 1 million characters. Same for the URL that handles the request - but there we only show the host, so there are practical limits to how long it can be.

> and how frequent this case might happen?

It's not about an actual website wanting to use a long protocol (where it's unlikely the protocol will be longer than 10-20 characters), it's about abuse. What happens if an abusive website puts a very long string in - so long that the popup stretches off-screen, and the buttons are invisible?

In other words, should we crop? Wrap? Something else? Limit to 100 characters and ellipse?

(The last option seems fine to me, but all of these require implementation work.)

Comment 95

[:Gijs (he/him)]

Comment on attachment 8818598 [details]
Bug 1270416 - registerProtocolHandler improvement with Permission UI and control center.  Amadini

https://reviewboard.mozilla.org/r/98620/#review102460

I don't understand why we need to do this manually. The PopupNotifications code used to take care of not showing multiple notifications at the same time. If that got broken IMO we should fix that instead - it would be equally unacceptable if geolocation and other notifications now appeared at the same time, and hacking around it by setting neverShow doesn't seem like an appropriate fix. Paolo, can you comment/review what is going on here, as I think you worked on the new permissions stuff?

::: browser/components/feeds/test/browser/browser_registerHandlerWithPermissionUI.js:46
(Diff revisions 4 - 5)
> +    // traverse all permissions and remove the permission we create in tests.
> +    let permissionKey = "handler-registration-" + testInfo.protocolType;

Shouldn't this replace the existing cleanup function?

::: browser/components/feeds/test/browser/browser_registerHandlerWithPermissionUI.js:322
(Diff revisions 4 - 5)
> +      // use getPopupNotificationNode to check there is only one notification on
> +      // the panel. we don't use clickButton because the notification panel will
> +      // show the next prompt directly after click any buttons.
> +      let notificationNode = getPopupNotificationNode();
> +      notificationNode.secondaryButton.click();
> +
> +      // use clickButton  to check there is only one notification on the panel.
> +      yield clickButton(false);

These comments and this code don't make sense when I'm just looking at this code / in the interdiff. Can you clarify them (and/or maybe make "clickButton" and "getPopupNotificationNode" as method names more descriptive) ? "clickButton" shouldn't be checking how many notifications are in the panel.

::: browser/modules/PermissionUI.jsm:695
(Diff revisions 4 - 5)
>                                  params.length);
>    },
>  
> +  get popupOptions() {
> +    let options = {};
> +    let notifications = this.request.browser.ownerGlobal.PopupNotifications._currentNotifications;

s/this.request.browser/this.browser/.

::: browser/modules/PermissionUI.jsm:697
(Diff revisions 4 - 5)
>  
> +  get popupOptions() {
> +    let options = {};
> +    let notifications = this.request.browser.ownerGlobal.PopupNotifications._currentNotifications;
> +    for (let notification of notifications) {
> +      if (notification.id.search("handler-registration") != -1 &&

Nit: notification.id.startsWith("handler-registration")

Comment 96

[Eden Chuang[:edenchuang]]

Comment on attachment 8818598 [details]
Bug 1270416 - registerProtocolHandler improvement with Permission UI and control center.  Amadini

https://reviewboard.mozilla.org/r/98620/#review102460

According to the code of PopupNotifications._update()

https://dxr.mozilla.org/mozilla-central/source/toolkit/modules/PopupNotifications.jsm#951

PopupNotifications seems not show only one notification at the same time. However, it shows only one anchor's notificaitons at one time. So notifications belongs to different anchors are not shown together. Although we give different anchorIDs for different type handler registration notifications, the xul element for these anchors does not exist, so these notifications still are set to a default anchor and  are shown together.

I am not sure if it is correct to limit one notification showing at one time. If this behavior is right, I think we should fire another bug to fix it.

> These comments and this code don't make sense when I'm just looking at this code / in the interdiff. Can you clarify them (and/or maybe make "clickButton" and "getPopupNotificationNode" as method names more descriptive) ? "clickButton" shouldn't be checking how many notifications are in the panel.

getPopupNotificationNode() is a function defined in head.js. It gets a notificaiton element from PopupNotificaitons.panel and check if there is only one notification in the panel.

clickButton() is a function defined in head.js. It calls getPopupNotificationNode() to get notification and click the "Always Allow" or "Never Allow" button, then wait for the popuphidden event fired on PopupNotifications.panel.

In this testcase, multiple notifications are sent into PopupNotificaiton. Because of our implementation, the "popuphidden" event will not be fired to PopupNotificaions.panel when notification button is clicked, so we cann't use clickButton to check there is only one notification is shown on the panel.

Comment 97

[Eden Chuang[:edenchuang]]

Attachment: Bug 1270416 - handler registration permission control integration with Control Center. Amadini

Review commit: https://reviewboard.mozilla.org/r/102600/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/102600/

Comment 98

[Eden Chuang[:edenchuang]]

Comment on attachment 8818598 [details]
Bug 1270416 - registerProtocolHandler improvement with Permission UI and control center.  Amadini

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/98620/diff/5-6/

Comment 99

[Eden Chuang[:edenchuang]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6c6333739c84

Comment 100

[Eden Chuang[:edenchuang]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=79b4f40b1eec

Comment 101

[Bryant Mao [:bryantmao]]

(In reply to :Gijs from comment #94)
> (In reply to Bryant Mao [:bryantmao] from comment #88)
> > (In reply to :Gijs (gone until 3 jan) from comment #66)
> > > What does the permission popup actually look like with the very long
> > > protocol at this stage - can you provide a screenshot and get UX review?
> > > 
> > To review the UI, I need to clarify two questions first. First, how long the
> > string can possibly be,
> 
> However long the website wants it to be. Could be 1 million characters. Same
> for the URL that handles the request - but there we only show the host, so
> there are practical limits to how long it can be.
> 
> > and how frequent this case might happen?
> 
> It's not about an actual website wanting to use a long protocol (where it's
> unlikely the protocol will be longer than 10-20 characters), it's about
> abuse. What happens if an abusive website puts a very long string in - so
> long that the popup stretches off-screen, and the buttons are invisible?
> 
> In other words, should we crop? Wrap? Something else? Limit to 100
> characters and ellipse?
> 
> (The last option seems fine to me, but all of these require implementation
> work.)

Thank for reminding the abuse case, I've discussed the issue with Eden, and we've come out a solution for the super long protocol link: we will abandon the website name and limited the website domain to 30 letters (Approximately to the default width of the door hanger). Here are the reasons behind:

Currently, there are three parts of the string is defined by the register website: 1) Website name; 2) Website domain 3) Protocol link type. 

Most of the protocol link types are quite short and are defined in a controlled pool from W3C so shouldn't be the primary cause of the case. The problem will lie in the unlimited website name and domain which creator can make them as long as they want. 

For the user, they just want to know "who" want to register the protocol link, and putting both the website name and domain seems not only redundant but also a bit confusing. Therefore, we decide only to keep the website domain since it usually has some meaning (ex. brand/function) and most creators will treat it much seriously as it is related to SEO. Moreover, a user should be able to tell/search the website by the first few letters of the domain (ex. mail.google.com), so we limited it to 30 letters to deal with the super long string.

We could have further discussion in another bug. Eden, what do you think?

Comment 102

[:Gijs (he/him)]

(In reply to Bryant Mao [:bryantmao] from comment #101)
> (In reply to :Gijs from comment #94)
> > (In reply to Bryant Mao [:bryantmao] from comment #88)
> > > (In reply to :Gijs (gone until 3 jan) from comment #66)
> > > > What does the permission popup actually look like with the very long
> > > > protocol at this stage - can you provide a screenshot and get UX review?
> > > > 
> > > To review the UI, I need to clarify two questions first. First, how long the
> > > string can possibly be,
> > 
> > However long the website wants it to be. Could be 1 million characters. Same
> > for the URL that handles the request - but there we only show the host, so
> > there are practical limits to how long it can be.
> > 
> > > and how frequent this case might happen?
> > 
> > It's not about an actual website wanting to use a long protocol (where it's
> > unlikely the protocol will be longer than 10-20 characters), it's about
> > abuse. What happens if an abusive website puts a very long string in - so
> > long that the popup stretches off-screen, and the buttons are invisible?
> > 
> > In other words, should we crop? Wrap? Something else? Limit to 100
> > characters and ellipse?
> > 
> > (The last option seems fine to me, but all of these require implementation
> > work.)
> 
> Thank for reminding the abuse case, I've discussed the issue with Eden, and
> we've come out a solution for the super long protocol link: we will abandon
> the website name and limited the website domain to 30 letters (Approximately
> to the default width of the door hanger). Here are the reasons behind:
> 
> Currently, there are three parts of the string is defined by the register
> website: 1) Website name; 2) Website domain 3) Protocol link type. 
> 
> Most of the protocol link types are quite short and are defined in a
> controlled pool from W3C so shouldn't be the primary cause of the case. The
> problem will lie in the unlimited website name and domain which creator can
> make them as long as they want. 

I do not believe this is true. Protocol link types are not currently limited in Firefox. You can call registerProtocolHandler with any arbitrary protocol string.

> For the user, they just want to know "who" want to register the protocol
> link, and putting both the website name and domain seems not only redundant
> but also a bit confusing. Therefore, we decide only to keep the website
> domain since it usually has some meaning (ex. brand/function) and most
> creators will treat it much seriously as it is related to SEO. Moreover, a
> user should be able to tell/search the website by the first few letters of
> the domain (ex. mail.google.com), so we limited it to 30 letters to deal
> with the super long string.

You haven't indicated how you intend to obtain the 30 letters. I would suggest the last (tailing part), to avoid spoofing risks ("myfancysubdomain.somewhere.google.com.somemorestuffthaticanregister.com" with the last bit chopped off looks like google.com). We should indicate cropping with leading ellipsis.

Comment 103

[:Paolo Amadini]

Comment on attachment 8818598 [details]
Bug 1270416 - registerProtocolHandler improvement with Permission UI and control center.  Amadini

https://reviewboard.mozilla.org/r/98620/#review103488

(In reply to :Gijs from comment #95)
> I don't understand why we need to do this manually. The PopupNotifications
> code used to take care of not showing multiple notifications at the same
> time. If that got broken IMO we should fix that instead - it would be
> equally unacceptable if geolocation and other notifications now appeared at
> the same time, and hacking around it by setting neverShow doesn't seem like
> an appropriate fix. Paolo, can you comment/review what is going on here, as
> I think you worked on the new permissions stuff?

If you just use the same notification ID, multiple notifications shouldn't show at the same time, as documented here:

https://developer.mozilla.org/en-US/docs/Mozilla/JavaScript_code_modules/PopupNotifications.jsm#show()

I agree that if that's broken in PopupNotifications.jsm for some reason, we should just fix that.

If we still need special code to serialize our registration requests, it should keep its own list of pending requests, and not just check which UI elements are displayed. The current solution causes a visible flicker when accepting a notification, you can see two notifications at the same time for a brief time.

We need an icon for the notification and the permission. As noted in comment 23, we can use something similar to Chrome's icon.

Comment 104

[:Paolo Amadini]

Comment on attachment 8824063 [details]
Bug 1270416 - handler registration permission control integration with Control Center.  Amadini

https://reviewboard.mozilla.org/r/102600/#review103490

This isn't right.

First, this doesn't show anything in the Control Center for protocols that aren't in a defined list, for example "myscheme", but we still store these permissions in the database and allow the protocols, the user just has no way to control them.

Second, we show every permission for every protocol on a separate line. It's simple to overload the Control Center by just calling the registration on all the whitelisted protocols.

Third, this adds a lot of redundant code for each whitelisted protocol instead of keeping a simple centralized list, so if a new protocol has to be whitelisted, a lot of code has to be added, and it's easy to forget to do that in every place. This is in addition to the maintenance cost of orthogonal changes to how permissions are handled.

When a permission is blocked, we also need to show its blocked version of the icon in the URL bar to indicate that this decision can be reverted from the Control Center.

Comment 105

[:Paolo Amadini]

(In reply to Bryant Mao [:bryantmao] from comment #101)
> we will abandon the website name

This refers to the third argument to registerProtocolHandler, and I agree that it should be ignored in the user interface to avoid spoofing since it's an arbitrary string provided by the website.

I think this should include the preferences page, in fact today "maliciouswebsite.com" can register itself as "GMail" and we'll happily show "GMail" in the preferences. This is currently slightly mitigated by the fact that we don't make this the default handler.

Comment 106

[:Paolo Amadini]

Actually, I was about to say that this can be a separate bug, you can file it if one doesn't already exist. It's smaller in scope and simplifies this bug, so you can consider implementing that bug before this one.

Comment 107

[:Paolo Amadini]

For the permission handling, this can make this bug much easier to implement:

1. Use a single permission ID instead of one for each protocol.
2. Don't store the Allow permission, but only Block.

Comment 108

[:Paolo Amadini]

For the security reasons I already explained extensively, we cannot set an arbitrary website as the default handler just by clicking on the blue "Allow" button in the popup notification.

We need to add a checkbox to the permission using the related PopupNotifications.jsm functionality, and only enable the Allow button after the checkbox has been selected. Google Chrome also requires a similar multi-click interaction before associating a protocol with a website.

Comment 109

[:Paolo Amadini]

(In reply to :Paolo Amadini from comment #107)
> 2. Don't store the Allow permission, but only Block.

In fact, in the current patch, clicking the "X" in the Control Center for an Allow permission removes the line but has no other effect. This is confusing because I'd expect this action to do something, in particular remove the association of the website as the default handler application.

This is, however, not very useful to do from the Control Center, and allows users to break their experience inadvertently. I think it's better to show nothing in the Control Center in the Allow case, and use the preferences to manage and remove the application if necessary.

Comment 110

[Eden Chuang[:edenchuang]]

(In reply to :Paolo Amadini from comment #103)
> Comment on attachment 8818598 [details]
> Bug 1270416 - Improve registerProtocolHandler API with PermissionUI. 
> Kruitbosch
> 
> https://reviewboard.mozilla.org/r/98620/#review103488
> 
> (In reply to :Gijs from comment #95)
> > I don't understand why we need to do this manually. The PopupNotifications
> > code used to take care of not showing multiple notifications at the same
> > time. If that got broken IMO we should fix that instead - it would be
> > equally unacceptable if geolocation and other notifications now appeared at
> > the same time, and hacking around it by setting neverShow doesn't seem like
> > an appropriate fix. Paolo, can you comment/review what is going on here, as
> > I think you worked on the new permissions stuff?
> 
> If you just use the same notification ID, multiple notifications shouldn't
> show at the same time, as documented here:
> 
> https://developer.mozilla.org/en-US/docs/Mozilla/JavaScript_code_modules/
> PopupNotifications.jsm#show()
> 

Yes, by using the same notifcationID, the new notification will replace the previous one. 
However, if we use the same notificationID for all handler registration notifications, it means user can make their own decision at the last registration, and other registration will be ignored until they visit the website at next time. 

> I agree that if that's broken in PopupNotifications.jsm for some reason, we
> should just fix that.
> 
> If we still need special code to serialize our registration requests, it
> should keep its own list of pending requests, and not just check which UI
> elements are displayed. The current solution causes a visible flicker when
> accepting a notification, you can see two notifications at the same time for
> a brief time.

If we needed, could we maintain this requests list in PopupNotifications? Because PopupNotifications considers lots of situations to maintain the notifications, if we can reuse these codes, we can easy to sync the status of the pending requests if some situations happens, for example, location changed and tab switched.  

> 
> We need an icon for the notification and the permission. As noted in comment
> 23, we can use something similar to Chrome's icon.

I will ask UX team to give a special icon for handler registration. 

> For the security reasons I already explained extensively, we cannot set an arbitrary website
> as the default handler just by clicking on the blue "Allow" button in the popup notification.
> We need to add a checkbox to the permission using the related PopupNotifications.jsm functionality,
> and only enable the Allow button after the checkbox has been selected. Google Chrome also requires a 
> similar multi-click interaction before associating a protocol with a website.

I will add a checkbox for it.

> For the permission handling, this can make this bug much easier to implement:
> 
> 1. Use a single permission ID instead of one for each protocol.
> 2. Don't store the Allow permission, but only Block.

Do you mean only using the permission mechanism to block notification showing?
If we do this, the UX spec seems need to be updated. For example, we should use "Allow" to replace "Always Allow", because the notification will be popup again once the default handler is changed.

Comment 111

[Eden Chuang[:edenchuang]]

(In reply to Bryant Mao [:bryantmao] from comment #101)
> (In reply to :Gijs from comment #94)
> > (In reply to Bryant Mao [:bryantmao] from comment #88)
> > > (In reply to :Gijs (gone until 3 jan) from comment #66)
> > > > What does the permission popup actually look like with the very long
> > > > protocol at this stage - can you provide a screenshot and get UX review?
> > > > 
> > > To review the UI, I need to clarify two questions first. First, how long the
> > > string can possibly be,
> > 
> > However long the website wants it to be. Could be 1 million characters. Same
> > for the URL that handles the request - but there we only show the host, so
> > there are practical limits to how long it can be.
> > 
> > > and how frequent this case might happen?
> > 
> > It's not about an actual website wanting to use a long protocol (where it's
> > unlikely the protocol will be longer than 10-20 characters), it's about
> > abuse. What happens if an abusive website puts a very long string in - so
> > long that the popup stretches off-screen, and the buttons are invisible?
> > 
> > In other words, should we crop? Wrap? Something else? Limit to 100
> > characters and ellipse?
> > 
> > (The last option seems fine to me, but all of these require implementation
> > work.)
> 
> Thank for reminding the abuse case, I've discussed the issue with Eden, and
> we've come out a solution for the super long protocol link: we will abandon
> the website name and limited the website domain to 30 letters (Approximately
> to the default width of the door hanger). Here are the reasons behind:
> 
> Currently, there are three parts of the string is defined by the register
> website: 1) Website name; 2) Website domain 3) Protocol link type. 
> 
> Most of the protocol link types are quite short and are defined in a
> controlled pool from W3C so shouldn't be the primary cause of the case. The
> problem will lie in the unlimited website name and domain which creator can
> make them as long as they want. 
> 
> For the user, they just want to know "who" want to register the protocol
> link, and putting both the website name and domain seems not only redundant
> but also a bit confusing. Therefore, we decide only to keep the website
> domain since it usually has some meaning (ex. brand/function) and most
> creators will treat it much seriously as it is related to SEO. Moreover, a
> user should be able to tell/search the website by the first few letters of
> the domain (ex. mail.google.com), so we limited it to 30 letters to deal
> with the super long string.
> 
> We could have further discussion in another bug. Eden, what do you think?

As comment 106 mentioned, I will fire a bug for it.

Comment 112

[:Paolo Amadini]

(In reply to Eden Chuang[:edenchuang] from comment #110)
> Yes, by using the same notifcationID, the new notification will replace the
> previous one. 
> However, if we use the same notificationID for all handler registration
> notifications, it means user can make their own decision at the last
> registration, and other registration will be ignored until they visit the
> website at next time.
> 
> If we needed, could we maintain this requests list in PopupNotifications?
> Because PopupNotifications considers lots of situations to maintain the
> notifications, if we can reuse these codes, we can easy to sync the status
> of the pending requests if some situations happens, for example, location
> changed and tab switched.  

These are good considerations, and I'm afraid this means that we still need special code here to serialize our registration requests. The PopupNotifications.jsm module isn't aware that the requests are related to a permission, and it cannot know that clicking "block" should deny all the other hidden requests, while clicking "allow" should show the next request in the queue.

> > For the permission handling, this can make this bug much easier to implement:
> > 
> > 1. Use a single permission ID instead of one for each protocol.
> > 2. Don't store the Allow permission, but only Block.
> 
> Do you mean only using the permission mechanism to block notification
> showing?
> If we do this, the UX spec seems need to be updated. For example, we should
> use "Allow" to replace "Always Allow", because the notification will be
> popup again once the default handler is changed.

Sorry, I don't fully understand this question, but I'll try to reply to the aspects it mentions.

If you're referring to the Control Center, likely we shouldn't show any "Allow" label because of the issues described in comment 109.

If you're referring to the notification prompts, for all other permissions we don't use the string "Always Allow" anymore, we just use "Allow" in all cases, and we should do the same here.

With regard to the case of changing the default handler:
1. Site A attempts to register as default application
2. User is prompted and chooses to allow site A
3. Site B attempts to replace the default application
4. User is prompted and chooses to allow site B
5. Site A attempts to register as default application again

One of the following should happen:
6a. User is not prompted, we show "Ask to handle application links" as "Blocked" in the Control Center
6b. User is prompted again, and they can choose to block or allow from the prompt (likely block)

This is unrelated to which label we use for the buttons in the prompt.

Comment 113

[Eden Chuang[:edenchuang]]

(In reply to :Paolo Amadini from comment #112)
> (In reply to Eden Chuang[:edenchuang] from comment #110)
> > Yes, by using the same notifcationID, the new notification will replace the
> > previous one. 
> > However, if we use the same notificationID for all handler registration
> > notifications, it means user can make their own decision at the last
> > registration, and other registration will be ignored until they visit the
> > website at next time.
> > 
> > If we needed, could we maintain this requests list in PopupNotifications?
> > Because PopupNotifications considers lots of situations to maintain the
> > notifications, if we can reuse these codes, we can easy to sync the status
> > of the pending requests if some situations happens, for example, location
> > changed and tab switched.  
> 
> These are good considerations, and I'm afraid this means that we still need
> special code here to serialize our registration requests. The
> PopupNotifications.jsm module isn't aware that the requests are related to a
> permission, and it cannot know that clicking "block" should deny all the
> other hidden requests, while clicking "allow" should show the next request
> in the queue.

According to UX design, yes, the special code is needed. Since the spec wants to prompt user for each protocol registration.
> 
> > > For the permission handling, this can make this bug much easier to implement:
> > > 
> > > 1. Use a single permission ID instead of one for each protocol.
> > > 2. Don't store the Allow permission, but only Block.
> > 
> > Do you mean only using the permission mechanism to block notification
> > showing?
> > If we do this, the UX spec seems need to be updated. For example, we should
> > use "Allow" to replace "Always Allow", because the notification will be
> > popup again once the default handler is changed.
> 
> Sorry, I don't fully understand this question, but I'll try to reply to the
> aspects it mentions.
> 
> If you're referring to the Control Center, likely we shouldn't show any
> "Allow" label because of the issues described in comment 109.
> 
> If you're referring to the notification prompts, for all other permissions
> we don't use the string "Always Allow" anymore, we just use "Allow" in all
> cases, and we should do the same here.

Yes, I was referring to the notification prompts. I said the UX need to be updated, because the UX spec uses term "Always Allow" not "Allow".

> 
> With regard to the case of changing the default handler:
> 1. Site A attempts to register as default application
> 2. User is prompted and chooses to allow site A
> 3. Site B attempts to replace the default application
> 4. User is prompted and chooses to allow site B
> 5. Site A attempts to register as default application again
> 
> One of the following should happen:
> 6a. User is not prompted, we show "Ask to handle application links" as
> "Blocked" in the Control Center
> 6b. User is prompted again, and they can choose to block or allow from the
> prompt (likely block)
> 
> This is unrelated to which label we use for the buttons in the prompt.

Yes, according to UX design, the situation 6.a will happen.

Comment 114

[Eden Chuang[:edenchuang]]

Comment on attachment 8818598 [details]
Bug 1270416 - registerProtocolHandler improvement with Permission UI and control center.  Amadini

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/98620/diff/6-7/

Comment 115

[Eden Chuang[:edenchuang]]

Comment on attachment 8818598 [details]
Bug 1270416 - registerProtocolHandler improvement with Permission UI and control center.  Amadini

https://reviewboard.mozilla.org/r/98618/#review113736

Comment 116

[Eden Chuang[:edenchuang]]

Hello Paolo

I wrote a patch according to your suggestions on comment 107.
I have already asked UX team to provide an icon for protocol handler registration, but the icon is still in design now. I wrote code for it, but use another icon, I think it should not affect the review. I will apply the new icon once the design is finished.

Comment 117

[Eden Chuang[:edenchuang]]

try server result for the reviewing patch 

https://treeherder.mozilla.org/#/jobs?repo=try&revision=aec2a0cfa72ed4c507db0cd18b83307dfbed2527&selectedJob=77127709

Comment 118

[:Paolo Amadini]

Thanks for the patch, I haven't had time to review it yet, but it looks like a good improvement over the previous version. I'll get to the review in the next few days.

Comment 119

[:Paolo Amadini]

I've tested the patch locally, although I've not looked at the code yet. The user interface looks good, but I found the following two bugs.

When a new site replaces an existing one, the "blocked" permission is not shown in the Control Center for the previous site, so it's impossible to revert the block. This is case 6a from comment 112.

When a site tries to register two protocols one immediately after the other, and the user accepts the first request, the second protocol is registered without asking, but we should ask for permission explicitly in that case. The block case works correctly though, because we block all other requests after the first is denied.

We should have regression tests specifically for these two cases.

Comment 120

[Eden Chuang[:edenchuang]]

(In reply to :Paolo Amadini from comment #119)
> I've tested the patch locally, although I've not looked at the code yet. The
> user interface looks good, but I found the following two bugs.
> 
> When a new site replaces an existing one, the "blocked" permission is not
> shown in the Control Center for the previous site, so it's impossible to
> revert the block. This is case 6a from comment 112.

According to your advices in comment 107.
  1. Use a single permission ID instead of one for each protocol.
  2. Don't store the Allow permission, but only Block.

6.a situation seems cannot be achieved.
Following is the case you mentioned in comment 112

  1. Site A attempts to register as default application
  2. User is prompted and chooses to allow site A
  3. Site B attempts to replace the default application
  4. User is prompted and chooses to allow site B
  5. Site A attempts to register as default application again

  6a. User is not prompted, we show "Ask to handle application links" as "Blocked" in the Control Center
  6b. User is prompted again, and they can choose to block or allow from the prompt (likely block)

At the step 4 when we set the site B as the default handler, we have no idea to know the relationship between Site A and the protocol, because we don't save "Allow" permission, therefore, we also can not set block permission for Site A.

Keep going to the step 5. 6b situation will not happen since site A is already in the application options for the protocol, that means site A is a possible handler in our database, and registerProtocolHandler() ends with isRegistered checking.

There is a chance we can achieve 6.a, if the following assumption is correct

  if the website is in the application options for the protocol, it means user must allow registerProtocolHandler() for the website in the past.

we can set the block permission for the website at the step 5 if it is an application option but not the default application. The only problem is I am not sure the assumption is correct, so I didn't implement it in the patch. How do you think?
 
> 
> When a site tries to register two protocols one immediately after the other,
> and the user accepts the first request, the second protocol is registered
> without asking, but we should ask for permission explicitly in that case.
> The block case works correctly though, because we block all other requests
> after the first is denied.

I think this should not happen. You can try the real case with https://irccloud.mozilla.com. The website tries to register protocol handler for irc/ircs protocol. Please make sure irccloud is not in your irc and ircs protocol handler options, because if irccloud is an application option for the protocol, registerProtocolHandler() will not prompt user anything. You can check and remove it in the preference page.

At the first time you visit the irccloud, you will see only one prompt, because we use the same notification ID for all handler-registration prompt, it makes the previous notification is replaced by the newest one. Please refer following code for details

  https://dxr.mozilla.org/mozilla-central/source/toolkit/modules/PopupNotifications.jsm#455

Click the "Allow" and you can see irccloud is set as the default application for one of irc or ircs, but the other one is "Always ask". When the second time you visit the website, you will see the prompt for the protocol which is not registered.

> 
> We should have regression tests specifically for these two cases.

For the second case, I have a test for it in my patch. For the first case, I can add a new test for it.

Comment 121

[:Paolo Amadini]

(In reply to Eden Chuang[:edenchuang] from comment #120)
> At the step 4 when we set the site B as the default handler, we have no idea
> to know the relationship between Site A and the protocol, because we don't
> save "Allow" permission, therefore, we also can not set block permission for
> Site A.

You can get the information by looking up the default handler for the protocol. The website for which we should set the block permission is the origin of the old default handler.

> Keep going to the step 5. 6b situation will not happen since site A is
> already in the application options for the protocol, that means site A is a
> possible handler in our database, and registerProtocolHandler() ends with
> isRegistered checking.

This explains why we don't ask again even if we haven't set the block permission. We have changed the meaning of the prompt to ask to set the default handler, and not only add the handler to the list of possible handlers. This means that the existing _protocolHandlerRegistered check is not relevant anymore, clearly we should just check whether the current site is the default handler or not.

Also, it just occurred to me that if the site is already in the list of possible handlers, and there is no block permission, we may want to update the URI template without asking anything to the user, if it has changed but it is still for the same origin. Advanced users who want to customize the template to something different than what the website offers may do so manually if there is a block permission for the website. Please file a separate bug to discuss this, if one isn't on file already.

> At the first time you visit the irccloud, you will see only one prompt,
> because we use the same notification ID for all handler-registration prompt,
> it makes the previous notification is replaced by the newest one.

The fact that PopupNotifications has this behavior is what I meant in comment 112 when I said that we need special code to keep a queue of registration requests _before_ we show them to the user with PopupNotifications.

> Click the "Allow" and you can see irccloud is set as the default application
> for one of irc or ircs, but the other one is "Always ask". When the second
> time you visit the website, you will see the prompt for the protocol which
> is not registered.

Ah, in my testing in comment 119 it seemed to me that the first registration was granted, but it actually just disappeared until the website asked again. This may encourage websites to ask repeatedly using a timer, which is probably not what we want.

That said, the current behavior in the patch is not too bad, and maybe we can work on serializing the requests in a follow-up. I'd like a second opinion from Gijs though, also about whether ensuring that we show multiple registration requests immediately should be a blocker for considering the feature complete.

Comment 122

[:Gijs (he/him)]

(In reply to :Paolo Amadini from comment #121)
> That said, the current behavior in the patch is not too bad, and maybe we
> can work on serializing the requests in a follow-up. I'd like a second
> opinion from Gijs though, also about whether ensuring that we show multiple
> registration requests immediately should be a blocker for considering the
> feature complete.

Sorry, I didn't get to this yesterday. I'm away today and will try to answer on Monday.

Comment 123

[:Gijs (he/him)]

(In reply to :Paolo Amadini from comment #121)
> (In reply to Eden Chuang[:edenchuang] from comment #120)
> > Click the "Allow" and you can see irccloud is set as the default application
> > for one of irc or ircs, but the other one is "Always ask". When the second
> > time you visit the website, you will see the prompt for the protocol which
> > is not registered.
> 
> That said, the current behavior in the patch is not too bad, and maybe we
> can work on serializing the requests in a follow-up. I'd like a second
> opinion from Gijs though, also about whether ensuring that we show multiple
> registration requests immediately should be a blocker for considering the
> feature complete.

I think dealing with multiple prompts in the 'allow' case (rather than the 'block' case) in a followup is fine.

Comment 124

[:Paolo Amadini]

Comment on attachment 8818598 [details]
Bug 1270416 - registerProtocolHandler improvement with Permission UI and control center.  Amadini

Sorry, this has been sitting in my queue for quite some time, but there are three actions pending in the meantime that we discussed:

- Fix _protocolHandlerRegistered and deny permission per comment 121
- File bug about updating registrations per comment 121
- File follow-up bug for comment 123

Comment 125

[:Paolo Amadini]

- Get the icon mentioned in comment 116
- Add the test mentioned in comment 120

Comment 126

[:Paolo Amadini]

Comment on attachment 8818598 [details]
Bug 1270416 - registerProtocolHandler improvement with Permission UI and control center.  Amadini

https://reviewboard.mozilla.org/r/98620/#review120042

::: browser/components/privatebrowsing/test/browser/browser_privatebrowsing_protocolhandler.js:9
(Diff revision 7)
>  
>  // This test makes sure that the web pages can't register protocol handlers
>  // inside the private browsing mode.
>  
>  add_task(function* test() {
> -  let notificationValue = "Protocol Registration: testprotocol";
> +  let notificationValue = "handler-registration";

Just use "handler-registration" directly in the code.

::: browser/components/privatebrowsing/test/browser/browser_privatebrowsing_protocolhandler.js:14
(Diff revision 7)
> -    let tab = aWindow.gBrowser.selectedTab = aWindow.gBrowser.addTab(testURI);
> -    yield BrowserTestUtils.browserLoaded(tab.linkedBrowser);
> +    let tab = aWindow.gBrowser.addTab("about:blank");
> +    let loadPromise = BrowserTestUtils.browserLoaded(tab.linkedBrowser);
> +    yield BrowserTestUtils.loadURI(tab.linkedBrowser, testURI);
> +    yield loadPromise;

You can use BrowserTestUtils.withNewTab.

::: browser/components/privatebrowsing/test/browser/browser_privatebrowsing_protocolhandler.js:20
(Diff revision 7)
> -    yield BrowserTestUtils.browserLoaded(tab.linkedBrowser);
> +    let loadPromise = BrowserTestUtils.browserLoaded(tab.linkedBrowser);
> +    yield BrowserTestUtils.loadURI(tab.linkedBrowser, testURI);
> +    yield loadPromise;
>  
> +    if (aIsPrivateMode) {
> -    let promiseFinished = PromiseUtils.defer();
> +      let promiseFinished = PromiseUtils.defer();

You can use:

yield new Promise(resolve => setTimeout(resolve, 100));

Using a timeout to check that the event isn't happening is not a great solution, but it's what the existing test did, so we can keep it for now.

::: browser/components/privatebrowsing/test/browser/browser_privatebrowsing_protocolhandler.js:30
(Diff revision 7)
> +      let shownPromise = BrowserTestUtils.waitForEvent(aWindow.PopupNotifications.panel, "popupshown");
> +      yield BrowserTestUtils.switchTab(aWindow.gBrowser, tab);
> +      yield shownPromise;

You should start waiting for the notification before the new is opened, otherwise you may incur in intermittent failures.

::: browser/locales/en-US/chrome/browser/feeds/subscribe.properties:48
(Diff revision 7)
>  # Protocol Handling
> -# "Add %appName (%appDomain) as an application for %protocolType links?"
> -addProtocolHandler=Add %S (%S) as an application for %S links?
> -addProtocolHandlerAddButton=Add Application
> -addProtocolHandlerAddButtonAccesskey=A
> +# for the case that default handler is not a web app.
> +# "Would you like %originalDefaultAppName to replace %appName (%appDomain) as the default application for %protocolType links?"
> +replaceDefaultProtocolHandler=Would you like %S to replace %S (%S) as the default application for %S links?
> +# for the case that default handler is a web app.
> +# "Would you like %originalDefaultAppName (%originalDefaultAppDomain) to replace %appName (%appDomain) as the default application for %protocolType links?"
> +replaceDefaultWebProtocolHandler=Would you like %S (%S) to replace %S (%S) as the default application for %S links?

These are swapped. In English, in the question "Would you like X to replace Y?", X is the new item and Y is the old item.

::: browser/locales/en-US/chrome/browser/sitePermissions.properties:38
(Diff revision 7)
>  permission.screen.label = Share the Screen
>  permission.install.label = Install Add-ons
>  permission.popup.label = Open Pop-up Windows
>  permission.geo.label = Access Your Location
>  permission.indexedDB.label = Maintain Offline Storage
> +permission.handler-registration.label = Set the Default Application for Protocols 

Whitespace at the end of line. Also, the strings need a copy review.

::: browser/modules/PermissionUI.jsm:304
(Diff revision 7)
> -            if (state && state.checkboxChecked) {
> +            if (state && state.checkboxChecked &&
> +                this.permissionKey != "handler-registration") {

Handler registration blocking should be permanent, otherwise we would ask again after the browser restarts.

Comment 127

[:Paolo Amadini]

Comment on attachment 8818598 [details]
Bug 1270416 - registerProtocolHandler improvement with Permission UI and control center.  Amadini

https://reviewboard.mozilla.org/r/98620/#review120052

::: browser/components/feeds/test/browser/browser_handlerRegistrationPermissionControl.js:5
(Diff revision 7)
> +"use strict";
> +
> +Cu.import("resource://gre/modules/Services.jsm");
> +
> +let gPermsSvc = Services.perms;

Just use Services.perms directly in the test.