502486 - Don't strip the www off urls for the block-image context menu label

Status: RESOLVED FIXED

(Firefox :: Menus, defect)

Description

[Nochum Sossonko [:Natch]]

From bug 417545 comment 91:

> The way nsPermissionManager handles entries is a bit complicated (although 
> not necessarily wrong). Any uri passed to it will get stored by host, as is. 
> Then when TestPermission is called, it will check the hash table for that 
> host and all of its' super-domains. In practice this means that 
> "http://www.google.com" won't match either "http://google.com" or 
> "http://images.google.com". However, "http://google.com" will match any 
> sub-domain of google.com, including "http://www.google.com" and 
> "http://images.google.com".

> The only thing wrong with that approach (and this probably belongs in a
> separate bug, which I'll file shortly) is that it doesn't cut off "www" which
> is not a sub-domain, rather a standard prefix. The intention is that the
> caller should be able to chose up to which sub-domain it wants to add the 
> permission for, and "www" shouldn't be considered a sub-domain in that case.

Bug filed.

Comment 1

[Nochum Sossonko [:Natch]]

Dan, can you offer any opinion here?

Gavin, nsContextMenu currently cuts off the www afaict to show in the context menu incorrectly, should this be fixed or should we just wait for a fix here?

Comment 2

[[e-mail]]

I don't really think we should do something like this in the backend - nsPermissionManager should provide a simple service of looking up a host and coming back with an answer. It shouldn't get into the business of guessing what the consumer wanted to do.

This is best handled in the front-end that deals with nsPermissionManager - i.e. in the Firefox code that adds the host in the first place. Do we not already have a regex in there somewhere to strip stuff off?

Comment 3

[Nochum Sossonko [:Natch]]

I was thinking the same thing originally, but then I thought if TestPermission does walk up the domains in the url already, then that means the permission manager is already dealing with domains, not plain input. As such I figured that to the permission manager's implementation www.google.com and google.com are really the same thing.

The browser code incorrectly uses the regex for the label, not when actually passing in the url, which is why if www.google.com is blocked, google.com won't be.

Comment 4

[[e-mail]]

(In reply to comment #3)
> I was thinking the same thing originally, but then I thought if TestPermission
> does walk up the domains in the url already, then that means the permission
> manager is already dealing with domains, not plain input. As such I figured
> that to the permission manager's implementation www.google.com and google.com
> are really the same thing.

To a browser, perhaps. nsPermissionManager is entirely unrelated to browsing, however... it's just a database of hosts that knows about the concept of subdomains.

Parsing out the host from a URI is trivially done with nsIURI, after which regexing off subdomains from |uri.host| is simple enough.

Comment 5

[Nochum Sossonko [:Natch]]

Ok, in that case I'll morph this into a browser bug and actually strip off the 'www'.

Comment 6

[Nochum Sossonko [:Natch]]

Gavin told me on irc to just change the label...

Comment 7

[Nochum Sossonko [:Natch]]

Attachment: label change

Comment 8

[Nochum Sossonko [:Natch]]

FTR, this was added in Firebird by one norrity. Obviously, as was the custom then, there's no bug or reason quoted...

Putting the cvs diff url in the url field for the curious...

Comment 9

[Nochum Sossonko [:Natch]]

Gavin: can you review this please, this patch is per your suggestion (I think).

Comment 10

[Nochum Sossonko [:Natch]]

Comment on attachment 387366 [details] [diff] [review]
label change

Come to think of it, maybe it's just better to keep the www as it doesn't really mean anything significant to the average user and adds clutter.

Alex, what are your thoughts?

Comment 11

[Alex Faaborg [:faaborg] (Firefox UX)]

From my quick reading of the previous comments it seems like stripping the www off helps both advanced users (since it is a more accurate representation of what will actually be blocked), as well as novice users (who don't really understand sub domain structures to begin with).

Comment 12

[Alex Faaborg [:faaborg] (Firefox UX)]

Comment on attachment 387366 [details] [diff] [review]
label change

I'm pretty sure I'm in favor of removing the www, as long as this matches the actual behavior correctly (so blocking images from a.site.com will also block images from b.site.com, and we should just list site.com)

Comment 13

[Nochum Sossonko [:Natch]]

Well, not exactly, the *actual* behavior is a bit different. In the specific case you mentioned, a.site.com won't be stripped down to site.com in the label. It's only www.site.com which gets stripped, which makes this entirely wrong, as www.site.com won't match site.com or a.site.com, only www.site.com.

The only reason I suggested not removing the www is because it _doesn't_ match the actual implementation (i.e. what actually happens).

For a more in-depth explanation see bug 417545 comment 91, which is what triggered this bug.

Comment 14

[Alex Faaborg [:faaborg] (Firefox UX)]

Comment on attachment 387366 [details] [diff] [review]
label change

>The only reason I suggested not removing the www is because it _doesn't_ match
>the actual implementation (i.e. what actually happens).

Ok, thanks for the clarification.  One way to try to make this more clear might be to add entries for both www.site.com and site.com to the exceptions window (and the user could later manager them separately, like choose to delete one).

In the meantime keeping the www sounds fine for the reasons you outlined.

Comment 15

[Alex Faaborg [:faaborg] (Firefox UX)]

>One way to try to make this more clear might
>be to add entries for both www.site.com and site.com to the exceptions window
>(and the user could later manager them separately, like choose to delete one).

To clarify, I meant if we decided to expand the blocking for the specific case of www.site.com to the full domain, this might make more sense in the exceptions window.

Comment 16

[Nochum Sossonko [:Natch]]

It's possible now to add whatever you want in the exceptions window, i.e. if you don't want to see any images at all from google you'd type in google.com in the input line and block that which will automatically block all images from any *.google.com site.

This is about the default accessible image-blocking menu entry you get in the image context-menu, which is probably the only substantial ui exposure this feature gets.

Comment 17

[Nochum Sossonko [:Natch]]

Comment on attachment 387366 [details] [diff] [review]
label change

Dao, can you review this? It's a tiny code removal that updates the context menu label to reality.

Comment 18

[Dão Gottwald [:dao]]

Comment on attachment 387366 [details] [diff] [review]
label change

var hostLabel; needs to be moved up a level to make it clear that it's used outside of the 'if' block.

Comment 19

[Nochum Sossonko [:Natch]]

Attachment: comment addressed

Comment 20

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/6874bc893f09

Comment 21

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Can we get a test for this change?