Adds a context message of the security origin for web notifications.

Adds a context message of the security origin for web notifications.

The context message should have the following properties:
* If the origin's scheme is http or https, omit it
* If the scheme is http or https and the port is default, omit it.
* Otherwise, format the entire URL according to net::FormatURL rules.

File origins have not been updated since it's currently not possible to get the notification permission for a file:// URL; a test has been added that will fail if this becomes possible.

BUG=402191
Committed: https://crrev.com/ec053615ba0c5dfe56f978437a045138f40ae9da
Cr-Commit-Position: refs/heads/master@{#315659}

[dewittj, 2014-10-16 22:14:52]

dewittj@chromium.org changed reviewers:
+ mukai@chromium.org, peter@rybin.spb.ru

[dewittj, 2014-10-16 22:15:09]

dewittj@chromium.org changed reviewers:
+ peter@chromium.org
- peter@rybin.spb.ru

[dewittj, 2014-10-16 22:16:56]

dewittj@chromium.org changed reviewers:
+ felt@chromium.org

[dewittj, 2014-10-16 22:16:57]

ptal
mukai, peter: please let me know if you think this is an appropriate location
for this annotation
felt: please look at the implementation of GURL->string16

[Jun Mukai, 2014-10-16 22:43:22]

lgtm

https://codereview.chromium.org/661643002/diff/1/chrome/browser/notifications...
File chrome/browser/notifications/desktop_notification_service.cc (right):

https://codereview.chromium.org/661643002/diff/1/chrome/browser/notifications...
chrome/browser/notifications/desktop_notification_service.cc:226:
rv.insert(rv.end(), port.begin(), port.end());
rv.append(base::UTF8ToUTF16(origin.port())) would be more concise and safer I
guess.

[Peter Beverloo, 2014-10-17 11:17:59]

https://codereview.chromium.org/661643002/diff/1/chrome/browser/notifications...
File chrome/browser/notifications/desktop_notification_service.cc (right):

https://codereview.chromium.org/661643002/diff/1/chrome/browser/notifications...
chrome/browser/notifications/desktop_notification_service.cc:222: base::string16
rv = net::IDNToUnicode(origin.host(), languages);
What does |rv| stand for? Can we use a clearer name, for example "domain"?

https://codereview.chromium.org/661643002/diff/1/chrome/browser/notifications...
chrome/browser/notifications/desktop_notification_service.cc:231: return
net::FormatUrl(GURL(displayed_origin), languages);
|displayed_origin| is guaranteed to be an empty string when we get here, that
can't be your intention.

[dewittj, 2014-10-17 18:24:13]

ptal, I renamed another unit test and added some specific tests for the origin
GURL -> display name code.

https://codereview.chromium.org/661643002/diff/1/chrome/browser/notifications...
File chrome/browser/notifications/desktop_notification_service.cc (right):

https://codereview.chromium.org/661643002/diff/1/chrome/browser/notifications...
chrome/browser/notifications/desktop_notification_service.cc:222: base::string16
rv = net::IDNToUnicode(origin.host(), languages);
On 2014/10/17 11:17:59, Peter Beverloo wrote:
> What does |rv| stand for? Can we use a clearer name, for example "domain"?

Done.

https://codereview.chromium.org/661643002/diff/1/chrome/browser/notifications...
chrome/browser/notifications/desktop_notification_service.cc:226:
rv.insert(rv.end(), port.begin(), port.end());
On 2014/10/16 22:43:22, Jun Mukai wrote:
> rv.append(base::UTF8ToUTF16(origin.port())) would be more concise and safer I
> guess.

Done.

https://codereview.chromium.org/661643002/diff/1/chrome/browser/notifications...
chrome/browser/notifications/desktop_notification_service.cc:231: return
net::FormatUrl(GURL(displayed_origin), languages);
On 2014/10/17 11:17:59, Peter Beverloo wrote:
> |displayed_origin| is guaranteed to be an empty string when we get here, that
> can't be your intention.

Yes, I had some uncommitted fixes in my tree that I managed not to upload :(

[Peter Beverloo, 2014-10-18 09:31:02]

lgtm

https://codereview.chromium.org/661643002/diff/40001/chrome/browser/notificat...
File chrome/browser/notifications/desktop_notification_service.cc (right):

https://codereview.chromium.org/661643002/diff/40001/chrome/browser/notificat...
chrome/browser/notifications/desktop_notification_service.cc:229: for
(extensions::ExtensionSet::const_iterator iter = extensions.begin();
nit: since you're moving this, might as well use C++11.

for (auto& extension : extensions) {
  NotifierId notifier_id(NotifierId::APPLICATION, extension->id());
  if (IsNotifierEnabled(notifier_id) {
    *out = base::UTF8ToUTF16(extension->name());
    return true;
  }
}

https://codereview.chromium.org/661643002/diff/40001/chrome/browser/notificat...
File chrome/browser/notifications/desktop_notification_service.h (right):

https://codereview.chromium.org/661643002/diff/40001/chrome/browser/notificat...
chrome/browser/notifications/desktop_notification_service.h:126: std::string
languages);
const std::string&

https://codereview.chromium.org/661643002/diff/40001/chrome/browser/notificat...
File chrome/browser/notifications/desktop_notification_service_unittest.cc
(right):

https://codereview.chromium.org/661643002/diff/40001/chrome/browser/notificat...
chrome/browser/notifications/desktop_notification_service_unittest.cc:18:
virtual void SetUp() { ChromeRenderViewHostTestHarness::SetUp(); }
SetUp() is defined in RenderViewHostTestHarness and should be in the vtbl, I
don't think there's a reason to override it here?

[dewittj, 2014-10-29 16:21:26]

https://codereview.chromium.org/661643002/diff/40001/chrome/browser/notificat...
File chrome/browser/notifications/desktop_notification_service.cc (right):

https://codereview.chromium.org/661643002/diff/40001/chrome/browser/notificat...
chrome/browser/notifications/desktop_notification_service.cc:229: for
(extensions::ExtensionSet::const_iterator iter = extensions.begin();
On 2014/10/18 09:31:02, Peter Beverloo wrote:
> nit: since you're moving this, might as well use C++11.
> 
> for (auto& extension : extensions) {
>   NotifierId notifier_id(NotifierId::APPLICATION, extension->id());
>   if (IsNotifierEnabled(notifier_id) {
>     *out = base::UTF8ToUTF16(extension->name());
>     return true;
>   }
> }

Done.

https://codereview.chromium.org/661643002/diff/40001/chrome/browser/notificat...
File chrome/browser/notifications/desktop_notification_service.h (right):

https://codereview.chromium.org/661643002/diff/40001/chrome/browser/notificat...
chrome/browser/notifications/desktop_notification_service.h:126: std::string
languages);
On 2014/10/18 09:31:02, Peter Beverloo wrote:
> const std::string&

I don't think we want to return a string reference since it's a local variable.

Additionally it should remain a string16 since it's going to be used in UI
right?

https://codereview.chromium.org/661643002/diff/40001/chrome/browser/notificat...
File chrome/browser/notifications/desktop_notification_service_unittest.cc
(right):

https://codereview.chromium.org/661643002/diff/40001/chrome/browser/notificat...
chrome/browser/notifications/desktop_notification_service_unittest.cc:18:
virtual void SetUp() { ChromeRenderViewHostTestHarness::SetUp(); }
On 2014/10/18 09:31:02, Peter Beverloo wrote:
> SetUp() is defined in RenderViewHostTestHarness and should be in the vtbl, I
> don't think there's a reason to override it here?

Done.

[Peter Beverloo, 2014-10-29 16:25:19]

https://codereview.chromium.org/661643002/diff/60001/chrome/browser/notificat...
File chrome/browser/notifications/desktop_notification_service.cc (right):

https://codereview.chromium.org/661643002/diff/60001/chrome/browser/notificat...
chrome/browser/notifications/desktop_notification_service.cc:195: int process_id
= render_frame_host->GetProcess()->GetID();
nit: You probably have to rebase this, since ShowDesktopNotification now takes a
|process_id| argument.

https://codereview.chromium.org/661643002/diff/60001/chrome/browser/notificat...
File chrome/browser/notifications/desktop_notification_service.h (right):

https://codereview.chromium.org/661643002/diff/60001/chrome/browser/notificat...
chrome/browser/notifications/desktop_notification_service.h:122: std::string
languages);
Sorry, I meant the |languages| argument, that can be a const ref.

[felt, 2014-10-30 06:40:14]

https://codereview.chromium.org/661643002/diff/60001/chrome/browser/notificat...
File chrome/browser/notifications/desktop_notification_service.cc (right):

https://codereview.chromium.org/661643002/diff/60001/chrome/browser/notificat...
chrome/browser/notifications/desktop_notification_service.cc:290: return
net::FormatUrl(origin, languages);
is it possible for other schemes to send notifications besides HTTP, HTTPS, or
file://? like... ftp?

[dewittj, 2014-11-03 17:18:33]

https://codereview.chromium.org/661643002/diff/60001/chrome/browser/notificat...
File chrome/browser/notifications/desktop_notification_service.cc (right):

https://codereview.chromium.org/661643002/diff/60001/chrome/browser/notificat...
chrome/browser/notifications/desktop_notification_service.cc:195: int process_id
= render_frame_host->GetProcess()->GetID();
On 2014/10/29 16:25:19, Peter Beverloo wrote:
> nit: You probably have to rebase this, since ShowDesktopNotification now takes
a
> |process_id| argument.

Done.

https://codereview.chromium.org/661643002/diff/60001/chrome/browser/notificat...
chrome/browser/notifications/desktop_notification_service.cc:290: return
net::FormatUrl(origin, languages);
On 2014/10/30 06:40:14, felt wrote:
> is it possible for other schemes to send notifications besides HTTP, HTTPS, or
> file://? like... ftp?

I'm not aware of anything preventing ftp:// origins.  However I guess it makes
sense to me only to omit http/https as ftp and others would be pretty unexpected
for notifications.

https://codereview.chromium.org/661643002/diff/60001/chrome/browser/notificat...
File chrome/browser/notifications/desktop_notification_service.h (right):

https://codereview.chromium.org/661643002/diff/60001/chrome/browser/notificat...
chrome/browser/notifications/desktop_notification_service.h:122: std::string
languages);
On 2014/10/29 16:25:19, Peter Beverloo wrote:
> Sorry, I meant the |languages| argument, that can be a const ref.

Done.

[felt, 2014-11-04 00:50:16]

https://codereview.chromium.org/661643002/diff/80001/chrome/browser/notificat...
File chrome/browser/notifications/desktop_notification_service.cc (right):

https://codereview.chromium.org/661643002/diff/80001/chrome/browser/notificat...
chrome/browser/notifications/desktop_notification_service.cc:182: // URL.  We
need a better way to display this than returning an empty string.
Could you have content/renderer/notification_provider.cc send over a full URL
instead of a SecurityOrigin if the SecurityOrigin is the empty string?

[dewittj, 2015-01-09 17:50:54]

Patchset #6 (id:100001) has been deleted

[dewittj, 2015-01-09 21:59:40]

peter: ptal for proper integration with your new class hierarchy here
felt: no functional change from previous patchset.

https://codereview.chromium.org/661643002/diff/80001/chrome/browser/notificat...
File chrome/browser/notifications/desktop_notification_service.cc (right):

https://codereview.chromium.org/661643002/diff/80001/chrome/browser/notificat...
chrome/browser/notifications/desktop_notification_service.cc:182: // URL.  We
need a better way to display this than returning an empty string.
On 2014/11/04 00:50:16, felt wrote:
> Could you have content/renderer/notification_provider.cc send over a full URL
> instead of a SecurityOrigin if the SecurityOrigin is the empty string?

This appears to be a broader issue than just notifications.  As such we'd rather
fix this for everything; see peter's recent mail to security-dev.

In the interim, can we ignore file:// urls entirely?(especially since it's
currently impossible to gain permission on file:// urls anyway)

[Peter Beverloo, 2015-01-10 00:03:33]

https://codereview.chromium.org/661643002/diff/130007/chrome/browser/notifica...
File chrome/browser/notifications/notification_browsertest.cc (right):

https://codereview.chromium.org/661643002/diff/130007/chrome/browser/notifica...
chrome/browser/notifications/notification_browsertest.cc:932:
IN_PROC_BROWSER_TEST_F(NotificationsTest, TestDisplayOriginContextMessage) {
This should probably go in PlatformNotificationBrowserTest.

https://codereview.chromium.org/661643002/diff/130007/chrome/browser/notifica...
File chrome/browser/notifications/platform_notification_service_impl.cc (right):

https://codereview.chromium.org/661643002/diff/130007/chrome/browser/notifica...
chrome/browser/notifications/platform_notification_service_impl.cc:257:
extensions::ExtensionSystem::Get(profile)->info_map();
This is the code that's violating threading constraints I was talking about.
extensions::InfoMap has a class-level comment that its methods may only be
called on the UI thread.

Given that we'll start doing security checks on the input per CL 834263004
already, I think that we can be a little bit less pedantic here about doing
another layer of validation. The only observable effect from all these checks
today is that we display a "weird" extension URL rather than an extension's
name.

We know that |origin| is an extension URL, so we can avoid doing the loop
altogether and instead just get the Extension* directly. We can also make the
assumption that the notifier is enabled, and DCHECK for that instead. After all,
this method only has to retrieve the extension's name.

What about the following?

==========
  const extensions::Extension* extension =
      extensions::ExtensionRegistry::Get(browser_context)->GetExtensionById(
          origin.host(),
          extensions::ExtensionRegistry::ENABLED);
  DCHECK(extension);

  return base::UTF8ToUTF16(extension->name());
==========

This approach:

    * Is safe to be called on the UI thread.
    * Can be in-lined into DisplayNameForOriginInProcessId, avoiding the need
for another method.
    * Avoids introducing a new dependencies on (a) Profile*, and (b)
DesktopNotificationService.
    * Allows the |render_process_id| argument to be removed from
Display(Persistent)Notification.

If desired, we can always DCHECK about the notifier being enabled here, but I
don't think we have to.

https://codereview.chromium.org/661643002/diff/130007/chrome/browser/notifica...
File chrome/browser/notifications/platform_notification_service_impl.h (right):

https://codereview.chromium.org/661643002/diff/130007/chrome/browser/notifica...
chrome/browser/notifications/platform_notification_service_impl.h:107: static
base::string16 OriginDisplayName(const GURL& origin,
Muh. I feel like this shouldn't be something in our class, there's a plethora of
features which would benefit from this. (A Push API TODO was added just today.)

An unsuccessful attempt was made in the following CL to add this to
//net/base/net_util.h:
  https://codereview.chromium.org/532523002/

Let me send an e-mail to a few folks for seeing if we can drive that to a
solution. Doesn't have to block this patch - happy to go forth one way or
another, it'll be easy to consolidate the paths.

[dewittj, 2015-01-13 21:02:23]

Patchset #8 (id:150001) has been deleted

[dewittj, 2015-01-13 21:02:36]

Patchset #8 (id:150001) has been deleted

[dewittj, 2015-01-13 22:18:01]

https://codereview.chromium.org/661643002/diff/130007/chrome/browser/notifica...
File chrome/browser/notifications/notification_browsertest.cc (right):

https://codereview.chromium.org/661643002/diff/130007/chrome/browser/notifica...
chrome/browser/notifications/notification_browsertest.cc:932:
IN_PROC_BROWSER_TEST_F(NotificationsTest, TestDisplayOriginContextMessage) {
On 2015/01/10 00:03:33, Peter Beverloo wrote:
> This should probably go in PlatformNotificationBrowserTest.

Done.

https://codereview.chromium.org/661643002/diff/130007/chrome/browser/notifica...
File chrome/browser/notifications/platform_notification_service_impl.cc (right):

https://codereview.chromium.org/661643002/diff/130007/chrome/browser/notifica...
chrome/browser/notifications/platform_notification_service_impl.cc:257:
extensions::ExtensionSystem::Get(profile)->info_map();
On 2015/01/10 00:03:33, Peter Beverloo wrote:
> This is the code that's violating threading constraints I was talking about.
> extensions::InfoMap has a class-level comment that its methods may only be
> called on the UI thread.
> 
> Given that we'll start doing security checks on the input per CL 834263004
> already, I think that we can be a little bit less pedantic here about doing
> another layer of validation. The only observable effect from all these checks
> today is that we display a "weird" extension URL rather than an extension's
> name.
> 
> We know that |origin| is an extension URL, so we can avoid doing the loop
> altogether and instead just get the Extension* directly. We can also make the
> assumption that the notifier is enabled, and DCHECK for that instead. After
all,
> this method only has to retrieve the extension's name.
> 
> What about the following?
> 
> ==========
>   const extensions::Extension* extension =
>       extensions::ExtensionRegistry::Get(browser_context)->GetExtensionById(
>           origin.host(),
>           extensions::ExtensionRegistry::ENABLED);
>   DCHECK(extension);
> 
>   return base::UTF8ToUTF16(extension->name());
> ==========
> 
> This approach:
> 
>     * Is safe to be called on the UI thread.
>     * Can be in-lined into DisplayNameForOriginInProcessId, avoiding the need
> for another method.
>     * Avoids introducing a new dependencies on (a) Profile*, and (b)
> DesktopNotificationService.
>     * Allows the |render_process_id| argument to be removed from
> Display(Persistent)Notification.
> 
> If desired, we can always DCHECK about the notifier being enabled here, but I
> don't think we have to.

Yes, I know this is the threading problem; just moving code around.

I'm happy to take your approach, although it appears that we already use methods
on Profile*: ->GetHostContentSettingsMap and (also from this patch ->GetPrefs) 
so we still have some work to do to remove all Profile* dereferences here.

Also prefer to use ExtensionRegistry::EVERYTHING so that we don't ever run into
a nullptr in non-debug code.  WDYT?

https://codereview.chromium.org/661643002/diff/130007/chrome/browser/notifica...
File chrome/browser/notifications/platform_notification_service_impl.h (right):

https://codereview.chromium.org/661643002/diff/130007/chrome/browser/notifica...
chrome/browser/notifications/platform_notification_service_impl.h:107: static
base::string16 OriginDisplayName(const GURL& origin,
On 2015/01/10 00:03:33, Peter Beverloo wrote:
> Muh. I feel like this shouldn't be something in our class, there's a plethora
of
> features which would benefit from this. (A Push API TODO was added just
today.)
> 
> An unsuccessful attempt was made in the following CL to add this to
> //net/base/net_util.h:
>   https://codereview.chromium.org/532523002/
> 
> Let me send an e-mail to a few folks for seeing if we can drive that to a
> solution. Doesn't have to block this patch - happy to go forth one way or
> another, it'll be easy to consolidate the paths.

right, that's the reason this patch has been in limbo.  Best to do it and
consolidate later :)

[Peter Beverloo, 2015-01-14 19:18:14]

LGTM, thanks!!

https://codereview.chromium.org/661643002/diff/130007/chrome/browser/notifica...
File chrome/browser/notifications/platform_notification_service_impl.cc (right):

https://codereview.chromium.org/661643002/diff/130007/chrome/browser/notifica...
chrome/browser/notifications/platform_notification_service_impl.cc:257:
extensions::ExtensionSystem::Get(profile)->info_map();
On 2015/01/13 22:18:01, dewittj wrote:
> On 2015/01/10 00:03:33, Peter Beverloo wrote:
> > This is the code that's violating threading constraints I was talking about.
> > extensions::InfoMap has a class-level comment that its methods may only be
> > called on the UI thread.
> > 
> > Given that we'll start doing security checks on the input per CL 834263004
> > already, I think that we can be a little bit less pedantic here about doing
> > another layer of validation. The only observable effect from all these
checks
> > today is that we display a "weird" extension URL rather than an extension's
> > name.
> > 
> > We know that |origin| is an extension URL, so we can avoid doing the loop
> > altogether and instead just get the Extension* directly. We can also make
the
> > assumption that the notifier is enabled, and DCHECK for that instead. After
> all,
> > this method only has to retrieve the extension's name.
> > 
> > What about the following?
> > 
> > ==========
> >   const extensions::Extension* extension =
> >       extensions::ExtensionRegistry::Get(browser_context)->GetExtensionById(
> >           origin.host(),
> >           extensions::ExtensionRegistry::ENABLED);
> >   DCHECK(extension);
> > 
> >   return base::UTF8ToUTF16(extension->name());
> > ==========
> > 
> > This approach:
> > 
> >     * Is safe to be called on the UI thread.
> >     * Can be in-lined into DisplayNameForOriginInProcessId, avoiding the
need
> > for another method.
> >     * Avoids introducing a new dependencies on (a) Profile*, and (b)
> > DesktopNotificationService.
> >     * Allows the |render_process_id| argument to be removed from
> > Display(Persistent)Notification.
> > 
> > If desired, we can always DCHECK about the notifier being enabled here, but
I
> > don't think we have to.
> 
> Yes, I know this is the threading problem; just moving code around.
> 
> I'm happy to take your approach, although it appears that we already use
methods
> on Profile*: ->GetHostContentSettingsMap and (also from this patch ->GetPrefs)

> so we still have some work to do to remove all Profile* dereferences here.
> 
> Also prefer to use ExtensionRegistry::EVERYTHING so that we don't ever run
into
> a nullptr in non-debug code.  WDYT?

That seems good to me. If this does ever happen in the wild, we should
understand why (disabled extensions shouldn't show notifications).

https://codereview.chromium.org/661643002/diff/130007/chrome/browser/notifica...
File chrome/browser/notifications/platform_notification_service_impl.h (right):

https://codereview.chromium.org/661643002/diff/130007/chrome/browser/notifica...
chrome/browser/notifications/platform_notification_service_impl.h:107: static
base::string16 OriginDisplayName(const GURL& origin,
On 2015/01/13 22:18:01, dewittj wrote:
> On 2015/01/10 00:03:33, Peter Beverloo wrote:
> > Muh. I feel like this shouldn't be something in our class, there's a
plethora
> of
> > features which would benefit from this. (A Push API TODO was added just
> today.)
> > 
> > An unsuccessful attempt was made in the following CL to add this to
> > //net/base/net_util.h:
> >   https://codereview.chromium.org/532523002/
> > 
> > Let me send an e-mail to a few folks for seeing if we can drive that to a
> > solution. Doesn't have to block this patch - happy to go forth one way or
> > another, it'll be easy to consolidate the paths.
> 
> right, that's the reason this patch has been in limbo.  Best to do it and
> consolidate later :)

Agreed.

https://codereview.chromium.org/661643002/diff/190001/chrome/browser/notifica...
File chrome/browser/notifications/platform_notification_service_impl.cc (right):

https://codereview.chromium.org/661643002/diff/190001/chrome/browser/notifica...
chrome/browser/notifications/platform_notification_service_impl.cc:262: 
micro nit: blank lines at EOF

https://codereview.chromium.org/661643002/diff/190001/chrome/browser/notifica...
File chrome/browser/notifications/platform_notification_service_impl.h (right):

https://codereview.chromium.org/661643002/diff/190001/chrome/browser/notifica...
chrome/browser/notifications/platform_notification_service_impl.h:100: static
base::string16 WebOriginDisplayName(const GURL& origin,
nit: TODO for future consolidation?

[dewittj, 2015-01-15 22:31:34]

dewittj@chromium.org changed reviewers:
+ avi@chromium.org

[dewittj, 2015-01-15 22:31:35]

+avi for content/

Adrienne, any opinions on landing the WebOriginDisplayName as-is?

https://codereview.chromium.org/661643002/diff/190001/chrome/browser/notifica...
File chrome/browser/notifications/platform_notification_service_impl.cc (right):

https://codereview.chromium.org/661643002/diff/190001/chrome/browser/notifica...
chrome/browser/notifications/platform_notification_service_impl.cc:262: 
On 2015/01/14 19:18:14, Peter Beverloo wrote:
> micro nit: blank lines at EOF

Done.

https://codereview.chromium.org/661643002/diff/190001/chrome/browser/notifica...
File chrome/browser/notifications/platform_notification_service_impl.h (right):

https://codereview.chromium.org/661643002/diff/190001/chrome/browser/notifica...
chrome/browser/notifications/platform_notification_service_impl.h:100: static
base::string16 WebOriginDisplayName(const GURL& origin,
On 2015/01/14 19:18:14, Peter Beverloo wrote:
> nit: TODO for future consolidation?

Done.

[felt, 2015-01-17 01:40:15]

https://codereview.chromium.org/661643002/diff/80001/chrome/browser/notificat...
File chrome/browser/notifications/desktop_notification_service.cc (right):

https://codereview.chromium.org/661643002/diff/80001/chrome/browser/notificat...
chrome/browser/notifications/desktop_notification_service.cc:182: // URL.  We
need a better way to display this than returning an empty string.
On 2015/01/09 21:59:40, dewittj wrote:
> On 2014/11/04 00:50:16, felt wrote:
> > Could you have content/renderer/notification_provider.cc send over a full
URL
> > instead of a SecurityOrigin if the SecurityOrigin is the empty string?
> 
> This appears to be a broader issue than just notifications.  As such we'd
rather
> fix this for everything; see peter's recent mail to security-dev.
> 
> In the interim, can we ignore file:// urls entirely?(especially since it's
> currently impossible to gain permission on file:// urls anyway)

Can you write a test to verify that it's impossible to gain the notification
permission on file:// urls, and put a comment on that test pointing here (& vice
versa)? That way if the logic is changed before this is fixed, we will get a
test failure.

[Avi (use Gerrit), 2015-01-17 03:23:55]

content lgtm

[dewittj, 2015-01-27 01:51:48]

felt, peter: I added a test that will fail and log if file:// URLs become able
to grant the notification permission; PTAL.

[Peter Beverloo, 2015-01-27 13:37:05]

Still lg % comment. Thanks for the update :-)

https://codereview.chromium.org/661643002/diff/270001/chrome/browser/notifica...
File chrome/browser/notifications/platform_notification_service_browsertest.cc
(right):

https://codereview.chromium.org/661643002/diff/270001/chrome/browser/notifica...
chrome/browser/notifications/platform_notification_service_browsertest.cc:126: }
 // namespace
micro nit:

namespace {
const char kTestFileName[] = "notifications/platform_notification_service.html";
}

[-blank line, -comment]

https://codereview.chromium.org/661643002/diff/270001/chrome/browser/notifica...
chrome/browser/notifications/platform_notification_service_browsertest.cc:274:
GURL file_url(net::FilePathToFileURL(full_file_path));
QQ: If server_root_ is "chrome/test/data" (which is relative) and
url::FilePathToFileURL prepends file:// and the CHDIR, which is the location of
the "chrome" binary, how do we end up being able to load the test file? Do we
create a symlink somewhere?

https://codereview.chromium.org/661643002/diff/270001/chrome/test/data/notifi...
File chrome/test/data/notifications/platform_notification_service.html (right):

https://codereview.chromium.org/661643002/diff/270001/chrome/test/data/notifi...
chrome/test/data/notifications/platform_notification_service.html:19:
domAutomationController.send(Notification.permission);
Are you doing this because |level| will say "granted" for file:// URLs, whereas
Notification.permission or checking the permission afterwards will return
"default"?

In that case, I'd prefer to check the value of Notification.permission
separately, since this kind of hides the fact that we're doing it wrong.

[dewittj, 2015-01-27 23:09:17]

Patchset #16 (id:330001) has been deleted

[dewittj, 2015-01-27 23:10:12]

https://codereview.chromium.org/661643002/diff/270001/chrome/browser/notifica...
File chrome/browser/notifications/platform_notification_service_browsertest.cc
(right):

https://codereview.chromium.org/661643002/diff/270001/chrome/browser/notifica...
chrome/browser/notifications/platform_notification_service_browsertest.cc:126: }
 // namespace
On 2015/01/27 13:37:04, Peter Beverloo wrote:
> micro nit:
> 
> namespace {
> const char kTestFileName[] =
"notifications/platform_notification_service.html";
> }
> 
> [-blank line, -comment]

Done.

https://codereview.chromium.org/661643002/diff/270001/chrome/browser/notifica...
chrome/browser/notifications/platform_notification_service_browsertest.cc:274:
GURL file_url(net::FilePathToFileURL(full_file_path));
On 2015/01/27 13:37:04, Peter Beverloo wrote:
> QQ: If server_root_ is "chrome/test/data" (which is relative) and
> url::FilePathToFileURL prepends file:// and the CHDIR, which is the location
of
> the "chrome" binary, how do we end up being able to load the test file? Do we
> create a symlink somewhere?

Right, looks like the try bots failed on that.  It worked on my development
machine because the current directory happened to be the source root.

https://codereview.chromium.org/661643002/diff/270001/chrome/test/data/notifi...
File chrome/test/data/notifications/platform_notification_service.html (right):

https://codereview.chromium.org/661643002/diff/270001/chrome/test/data/notifi...
chrome/test/data/notifications/platform_notification_service.html:19:
domAutomationController.send(Notification.permission);
On 2015/01/27 13:37:04, Peter Beverloo wrote:
> Are you doing this because |level| will say "granted" for file:// URLs,
whereas
> Notification.permission or checking the permission afterwards will return
> "default"?
> 
> In that case, I'd prefer to check the value of Notification.permission
> separately, since this kind of hides the fact that we're doing it wrong.

Yes, that's correct.  I've reimplemented this by reverting the html file and
using the DesktopNotificationService to check permissions; does this seem OK to
you?  Or would you prefer a CheckStaticNotificationPermission() function within
the JS that I call separately?

[Peter Beverloo, 2015-01-28 10:21:41]

On 2015/01/27 23:10:12, dewittj wrote:
>
https://codereview.chromium.org/661643002/diff/270001/chrome/browser/notifica...
> File chrome/browser/notifications/platform_notification_service_browsertest.cc
> (right):
> 
>
https://codereview.chromium.org/661643002/diff/270001/chrome/browser/notifica...
> chrome/browser/notifications/platform_notification_service_browsertest.cc:126:
}
>  // namespace
> On 2015/01/27 13:37:04, Peter Beverloo wrote:
> > micro nit:
> > 
> > namespace {
> > const char kTestFileName[] =
> "notifications/platform_notification_service.html";
> > }
> > 
> > [-blank line, -comment]
> 
> Done.
> 
>
https://codereview.chromium.org/661643002/diff/270001/chrome/browser/notifica...
> chrome/browser/notifications/platform_notification_service_browsertest.cc:274:
> GURL file_url(net::FilePathToFileURL(full_file_path));
> On 2015/01/27 13:37:04, Peter Beverloo wrote:
> > QQ: If server_root_ is "chrome/test/data" (which is relative) and
> > url::FilePathToFileURL prepends file:// and the CHDIR, which is the location
> of
> > the "chrome" binary, how do we end up being able to load the test file? Do
we
> > create a symlink somewhere?
> 
> Right, looks like the try bots failed on that.  It worked on my development
> machine because the current directory happened to be the source root.
> 
>
https://codereview.chromium.org/661643002/diff/270001/chrome/test/data/notifi...
> File chrome/test/data/notifications/platform_notification_service.html
(right):
> 
>
https://codereview.chromium.org/661643002/diff/270001/chrome/test/data/notifi...
> chrome/test/data/notifications/platform_notification_service.html:19:
> domAutomationController.send(Notification.permission);
> On 2015/01/27 13:37:04, Peter Beverloo wrote:
> > Are you doing this because |level| will say "granted" for file:// URLs,
> whereas
> > Notification.permission or checking the permission afterwards will return
> > "default"?
> > 
> > In that case, I'd prefer to check the value of Notification.permission
> > separately, since this kind of hides the fact that we're doing it wrong.
> 
> Yes, that's correct.  I've reimplemented this by reverting the html file and
> using the DesktopNotificationService to check permissions; does this seem OK
to
> you?  Or would you prefer a CheckStaticNotificationPermission() function
within
> the JS that I call separately?

That's fine with me, thanks. The JavaScript getter is going to return exactly
that, which is well covered by other tests.

[felt, 2015-01-29 20:22:44]

Thanks for doing this.

https://codereview.chromium.org/661643002/diff/350001/chrome/browser/notifica...
File chrome/browser/notifications/platform_notification_service_impl.cc (right):

https://codereview.chromium.org/661643002/diff/350001/chrome/browser/notifica...
chrome/browser/notifications/platform_notification_service_impl.cc:256: }
I'm fine with dropping https:// because that is meant to be a safe default.

However, can you please retain the http:// for http sites?

[dewittj, 2015-02-02 19:42:58]

Not a url parsing expert, but added the scheme the same way that FormatURL does;
using CountCharactersBefore(USERNAME).

https://codereview.chromium.org/661643002/diff/350001/chrome/browser/notifica...
File chrome/browser/notifications/platform_notification_service_impl.cc (right):

https://codereview.chromium.org/661643002/diff/350001/chrome/browser/notifica...
chrome/browser/notifications/platform_notification_service_impl.cc:256: }
On 2015/01/29 20:22:44, felt wrote:
> I'm fine with dropping https:// because that is meant to be a safe default.
> 
> However, can you please retain the http:// for http sites?

Done.

[felt, 2015-02-04 10:45:34]

On 2015/02/02 19:42:58, dewittj wrote:
> Not a url parsing expert, but added the scheme the same way that FormatURL
does;
> using CountCharactersBefore(USERNAME).
> 
>
https://codereview.chromium.org/661643002/diff/350001/chrome/browser/notifica...
> File chrome/browser/notifications/platform_notification_service_impl.cc
(right):
> 
>
https://codereview.chromium.org/661643002/diff/350001/chrome/browser/notifica...
> chrome/browser/notifications/platform_notification_service_impl.cc:256: }
> On 2015/01/29 20:22:44, felt wrote:
> > I'm fine with dropping https:// because that is meant to be a safe default.
> > 
> > However, can you please retain the http:// for http sites?
> 
> Done.

thank you, lgtm

[dewittj, 2015-02-10 19:19:15]

New patchsets have been uploaded after l-g-t-m from felt@chromium.org

[dewittj, 2015-02-10 21:46:26]

The CQ bit was checked by dewittj@chromium.org

[commit-bot: I haz the power, 2015-02-10 21:47:43]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/661643002/380001

[commit-bot: I haz the power, 2015-02-10 22:34:24]

Committed patchset #18 (id:380001)

[commit-bot: I haz the power, 2015-02-10 22:35:14]

Patchset 18 (id:??) landed as
https://crrev.com/ec053615ba0c5dfe56f978437a045138f40ae9da
Cr-Commit-Position: refs/heads/master@{#315659}