Closed Bug 1215409 Opened 4 years ago Closed 4 years ago

Add back GetSourceHostPort and remove duplicate permissions removal code that landed with 1209602

Categories

(Toolkit :: Notifications and Alerts, defect)

defect
Not set

Tracking

()

RESOLVED DUPLICATE of bug 1209602

People

(Reporter: jaws, Assigned: jaws)

References

Details

Comments from bug 1209602.

(In reply to Matthew N. [:MattN] (PTO Oct. 8-12) from comment #12)
> Jared, I don't understand the benefit of getting rid of GetSource and
> GetSourceHostPort? We should have exposed it to XUL notifications via XPIDL
> instead of removing it and duplicating the logic 3 times.

(In reply to Matthew N. [:MattN] (PTO Oct. 8-12) from comment #13)
> Comment on attachment 8672805 [details] [diff] [review]
> Patch with test (test file added to patch now)
> 
> Review of attachment 8672805 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/components/alerts/resources/content/alert.js
> @@ +238,5 @@
> >  
> > +function disableForOrigin() {
> > +  if (gPrincipal) {
> > +    Services.perms.removeFromPrincipal(gPrincipal,
> > +                                       "desktop-notification");
> 
> I totally missed the fact that you were duplicating existing logic. You
> should have observed "alertdisablecallback" instead. Then you wouldn't have
> had to change AlertUtils and arguments[10].
> 
> if (gAlertListener) {
>   gAlertListener.observe(null, "alertdisablecallback", gAlertCookie);
> }

(In reply to Matthew N. [:MattN] (PTO Oct. 8-12) from comment #14)
> (In reply to Matthew N. [:MattN] (PTO Oct. 8-12) from comment #13)
> > if (gAlertListener) {
> >   gAlertListener.observe(null, "alertdisablecallback", gAlertCookie);
> > }
> 
> You actually shouldn't need the `if` here for the same reason as what I said
> about `if (gPrincipal)`
Bug 1209602 got backed out so I'll just do the work there.
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1209602
You need to log in before you can comment on or make changes to this bug.