Example: https://cs.chromium.org/chromium/src/chrome/browser/ui/webui/engagement/site_engagement_ui.cc?rcl=0&l=58

This has been considered but I think the content settings system which we use for data storage doesn't support url::Origins yet.

raymes@: thoughts?

Yeah it's unclear what to do with content settings as we currently allow file:// URLs to be stored with the whole path as well as chrome-extension:// URLs. I think we will lose necessary information if we go through url::Origin (maybe not for chrome-extension, I'm not sure).

There is a larger question of whether we should be allowing settings to be stored for these schemes but until we decide upon that we wouldn't be able to change it I think. 

Since site engagement should only work for http(s) schemes, we could use a url::Origin there go back to a GURL for content settings but I don't feel strongly about that. dcheng@ you might have some thoughts?

I haven't thought about this a lot to be honest. It's just kind of a reflex for me to see "GURL origin" and be surprised.

If content settings doesn't support this though, I guess we're kind of stuck until we decide we want to change how this works.

Alternatively we could choose to retain file path information in a url::Origin. We wouldn't change how we compare origins but it may be useful in situations like this (if there are similar situations). 

+mkwst for thoughts

Raymes, just to be clear, what are file and chrome extension URLs used for in content settings?

For file URLs, I assume it is to persist permission decisions for web pages served on the file system. Is that right?

Is it the same thing for chrome extension URLs?

What do content settings do with `file:` URLs? I skimmed the patch, but didn't follow the value all the way through.

Re #6: It's definitely not used for permissions like geolocation. These just don't work on file:// URLs. Things like plugins do though (e.g. you can allow plugins for a file:// URL and it will get stored).

I'm not sure about chrome-extensions. I'll try to add some instrumentation to a build so we can find out when those values get passed. We definitely whitelist some content settings for chrome-extensions schemes, e.g. image and javascript.

+palmer

Re: chrome extension URLs, seems like different extensions should be different origins. Maybe we could tweak url::Origin to enforce this.

Re: file URLs, I think Mozilla treat different files as different origins, maybe we could do the same.

If that's fine that would work regardless of exactly how we're using these in the content settings.

mkwst / palmer: is updating the origin class like this crazy talk?

We also do (or, should) treat different file pathnames as different URLs. If we don't, that's a nasty vulnerability.

I think your plan to treat different extensions as different in url::Origin is good. Thanks!

> Re: chrome extension URLs, seems like different extensions should be different origins

+1 - I think this should work with the regular way that origins are parsed, we would just need to add support for the chrome-extension scheme to url::Origin. Currently we only allow "standard" schemes to be parsed. palmer/mkwst: would you support adding chrome-extensions scheme support?

> Re: file URLs, I think Mozilla treat different files as different origins, maybe we could do the same.

I think we sort of do this - file:// urls get parsed as unique origins which will never compare equal with any other origin. But we don't retain any path information and a file:// origin will never compare equal with itself. 

palmer/mkwst - how about retaining path information for a file URL in an url::Origin and allowing equivalent file URLs to compare equal? Would that break something?

raymes: That seems reasonable to me.

I think lshang@ will try to take a look this quarter.

Re #6 - what are they used for?

Chrome extensions can be whitelisted for some content settings (as Raymes pointed out in #8), but still respect most others. Basically run-time permissions for Chrome's apps & extensions.

The most prominent usage of file:// exceptions that I have seen (through various bug reports) was to open local PDF files but not ones on the web.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/d06a12d4cf219e1c310cdc736736f35d6168de49

commit d06a12d4cf219e1c310cdc736736f35d6168de49
Author: lshang <lshang@chromium.org>
Date: Thu Aug 11 03:09:00 2016

Add metrics for schemes of content setting exceptions

Add a uma metric to collect data for schemes (http/https/file/chrome-extension) of
content setting exceptions.

BUG=621724

Review-Url: https://codereview.chromium.org/2226643002
Cr-Commit-Position: refs/heads/master@{#411245}

[modify] https://crrev.com/d06a12d4cf219e1c310cdc736736f35d6168de49/components/content_settings/core/browser/host_content_settings_map.cc
[modify] https://crrev.com/d06a12d4cf219e1c310cdc736736f35d6168de49/components/content_settings/core/browser/host_content_settings_map.h
[modify] https://crrev.com/d06a12d4cf219e1c310cdc736736f35d6168de49/components/content_settings/core/common/content_settings_pattern.cc
[modify] https://crrev.com/d06a12d4cf219e1c310cdc736736f35d6168de49/components/content_settings/core/common/content_settings_pattern.h
[modify] https://crrev.com/d06a12d4cf219e1c310cdc736736f35d6168de49/components/content_settings/core/common/content_settings_pattern_unittest.cc
[modify] https://crrev.com/d06a12d4cf219e1c310cdc736736f35d6168de49/tools/metrics/histograms/histograms.xml

Requesting merge of #16 to M53. It's a simple change to add metrics of user's content setting exception schemes and it won't change any existing behavior, so it should be pretty safe to merge, and we can get data from stable users in shorter time.

Your change meets the bar and is auto-approved for M53 (branch: 2785)

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/0236e0743e89ca13a5b558f8a282f859e52589e7

commit 0236e0743e89ca13a5b558f8a282f859e52589e7
Author: Matt Giuca <mgiuca@chromium.org>
Date: Wed Aug 17 07:50:20 2016

Add metrics for schemes of content setting exceptions

Add a uma metric to collect data for schemes (http/https/file/chrome-extension) of
content setting exceptions.

BUG=621724

Review-Url: https://codereview.chromium.org/2226643002
Cr-Commit-Position: refs/heads/master@{#411245}
(cherry picked from commit d06a12d4cf219e1c310cdc736736f35d6168de49)

Review URL: https://codereview.chromium.org/2250713003 .

Cr-Commit-Position: refs/branch-heads/2785@{#637}
Cr-Branched-From: 68623971be0cfc492a2cb0427d7f478e7b214c24-refs/heads/master@{#403382}

[modify] https://crrev.com/0236e0743e89ca13a5b558f8a282f859e52589e7/components/content_settings/core/browser/host_content_settings_map.cc
[modify] https://crrev.com/0236e0743e89ca13a5b558f8a282f859e52589e7/components/content_settings/core/browser/host_content_settings_map.h
[modify] https://crrev.com/0236e0743e89ca13a5b558f8a282f859e52589e7/components/content_settings/core/common/content_settings_pattern.cc
[modify] https://crrev.com/0236e0743e89ca13a5b558f8a282f859e52589e7/components/content_settings/core/common/content_settings_pattern.h
[modify] https://crrev.com/0236e0743e89ca13a5b558f8a282f859e52589e7/components/content_settings/core/common/content_settings_pattern_unittest.cc
[modify] https://crrev.com/0236e0743e89ca13a5b558f8a282f859e52589e7/tools/metrics/histograms/histograms.xml

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/37df8891dff743a4a693575058ce22ca22adbe2a

commit 37df8891dff743a4a693575058ce22ca22adbe2a
Author: alexmos <alexmos@chromium.org>
Date: Tue Sep 27 17:27:08 2016

Add metrics for paths in file scheme exceptions in content settings.

These will tell how often file scheme exceptions have a non-empty
path vs. a wildcard path, and which content settings types use them.

BUG=628759, 621724

Review-Url: https://codereview.chromium.org/2367683002
Cr-Commit-Position: refs/heads/master@{#421248}

[modify] https://crrev.com/37df8891dff743a4a693575058ce22ca22adbe2a/components/content_settings/core/browser/host_content_settings_map.cc
[modify] https://crrev.com/37df8891dff743a4a693575058ce22ca22adbe2a/components/content_settings/core/common/content_settings_pattern.cc
[modify] https://crrev.com/37df8891dff743a4a693575058ce22ca22adbe2a/components/content_settings/core/common/content_settings_pattern.h
[modify] https://crrev.com/37df8891dff743a4a693575058ce22ca22adbe2a/components/content_settings/core/common/content_settings_pattern_unittest.cc
[modify] https://crrev.com/37df8891dff743a4a693575058ce22ca22adbe2a/tools/metrics/histograms/histograms.xml

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/a60ed7ff6c56075b5b3ae018d527f397c2869ca9

commit a60ed7ff6c56075b5b3ae018d527f397c2869ca9
Author: Alex Moshchuk <alexmos@chromium.org>
Date: Wed Sep 28 18:33:22 2016

Add metrics for paths in file scheme exceptions in content settings.

These will tell how often file scheme exceptions have a non-empty
path vs. a wildcard path, and which content settings types use them.

BUG=628759, 621724

Review-Url: https://codereview.chromium.org/2367683002
Cr-Commit-Position: refs/heads/master@{#421248}
(cherry picked from commit 37df8891dff743a4a693575058ce22ca22adbe2a)

Review URL: https://codereview.chromium.org/2375183002 .

Cr-Commit-Position: refs/branch-heads/2840@{#566}
Cr-Branched-From: 1ae106dbab4bddd85132d5b75c670794311f4c57-refs/heads/master@{#414607}

[modify] https://crrev.com/a60ed7ff6c56075b5b3ae018d527f397c2869ca9/components/content_settings/core/browser/host_content_settings_map.cc
[modify] https://crrev.com/a60ed7ff6c56075b5b3ae018d527f397c2869ca9/components/content_settings/core/common/content_settings_pattern.cc
[modify] https://crrev.com/a60ed7ff6c56075b5b3ae018d527f397c2869ca9/components/content_settings/core/common/content_settings_pattern.h
[modify] https://crrev.com/a60ed7ff6c56075b5b3ae018d527f397c2869ca9/components/content_settings/core/common/content_settings_pattern_unittest.cc
[modify] https://crrev.com/a60ed7ff6c56075b5b3ae018d527f397c2869ca9/tools/metrics/histograms/histograms.xml

Is this likely to ship for 55?

I ask because currently, file:// URL content exceptions don't work correctly for Flash.

The plugins code all uses url::Origins already, and when users add an exception that looks like file:///foo/bar, it doesn't work for the actual path file:///foo/bar, since it's converted to just file:// before content settings is queried.

Tommy

file:///* does work correctly, just specific paths don't work as expected.

#22: This is still under discussion and blocked by file:// URLs. Some metrics are added recently to provide some reference of file scheme exceptions usage.

Would it hurt much for plugins if we get rid of file:// URL exceptions?

Re #22 - is that a recent regression? These used to work. 

We won't be shipping this for M55

You're right - this seems broken, even on Stable. I'm pretty sure it did work at some point. But I guess the fact it's broken and no one noticed works in our favor for deprecating the behavior.

#26: I'm guessing this is due to my r345104, so this might have been around since M47.  We discussed making this change in https://groups.google.com/a/chromium.org/d/msg/site-isolation-dev/JPKPFfZpVHE/cS6covBqBgAJ, and no one objected to it at the time.

Re #22: Oh, so that's what it was! I have been debugging that before, and didn't see any problem in content settings...

Re #24: I know about one bug from an admin trying to use them in a policy. I guess corporate environments can have peculiar setups. But I don't know for how long it's relevant with all the deprecations going on?

So I would reiterate #9 and #11 - I think everyone agrees that every file path is its own origin, so as long as the file:// scheme exists, should we update url::Origin? mkwst@?

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/a60ed7ff6c56075b5b3ae018d527f397c2869ca9

commit a60ed7ff6c56075b5b3ae018d527f397c2869ca9
Author: Alex Moshchuk <alexmos@chromium.org>
Date: Wed Sep 28 18:33:22 2016

Add metrics for paths in file scheme exceptions in content settings.

These will tell how often file scheme exceptions have a non-empty
path vs. a wildcard path, and which content settings types use them.

BUG=628759, 621724

Review-Url: https://codereview.chromium.org/2367683002
Cr-Commit-Position: refs/heads/master@{#421248}
(cherry picked from commit 37df8891dff743a4a693575058ce22ca22adbe2a)

Review URL: https://codereview.chromium.org/2375183002 .

Cr-Commit-Position: refs/branch-heads/2840@{#566}
Cr-Branched-From: 1ae106dbab4bddd85132d5b75c670794311f4c57-refs/heads/master@{#414607}

[modify] https://crrev.com/a60ed7ff6c56075b5b3ae018d527f397c2869ca9/components/content_settings/core/browser/host_content_settings_map.cc
[modify] https://crrev.com/a60ed7ff6c56075b5b3ae018d527f397c2869ca9/components/content_settings/core/common/content_settings_pattern.cc
[modify] https://crrev.com/a60ed7ff6c56075b5b3ae018d527f397c2869ca9/components/content_settings/core/common/content_settings_pattern.h
[modify] https://crrev.com/a60ed7ff6c56075b5b3ae018d527f397c2869ca9/components/content_settings/core/common/content_settings_pattern_unittest.cc
[modify] https://crrev.com/a60ed7ff6c56075b5b3ae018d527f397c2869ca9/tools/metrics/histograms/histograms.xml

This issue has been Available for over a year. If it's no longer important or seems unlikely to be fixed, please consider closing it out. If it is important, please re-triage the issue.

Sorry for the inconvenience if the bug really should have been left as Available. If you change it back, also remove the "Hotlist-Recharge-Cold" label.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

This is still work that needs to be done, removing Hotlist-Recharge-Cold.