Remove the strict dependency on WebContents for Web Notifications.

Remove the strict dependency on WebContents for Web Notifications.

This patch changes Notification objects created for Web Notifications to
go without an associated WebContents. Because we want Web Notifications
to be used from Web Workers, this no longer is a valid assumption to make.

BUG=415160
Committed: https://crrev.com/b3024230575987f177b860a189551099f56ea50b
Cr-Commit-Position: refs/heads/master@{#301394}

[Peter Beverloo, 2014-09-17 16:24:07]

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

[Peter Beverloo, 2014-09-17 16:24:08]

This patch depends on https://codereview.chromium.org/554213003/ (without it,
Web Notification icons are no longer being fetched).

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

https://codereview.chromium.org/578883003/diff/1/chrome/browser/notifications...
chrome/browser/notifications/desktop_notification_service.cc:171: base::string16
DesktopNotificationService::DisplayNameForOrigin(
This mimics the behavior of GetExtensionsWithAPIPermissionForSecurityOrigin in
the origin.SchemeIs(extensions://) case, but with the exception that it does not
check the process Id.

Why do we have to check the process Id for the relevant extensions if we already
know they have permission to run in whatever that process is? If we do need the
process Id, we can definitely pass it through the API.

[dewittj, 2014-09-17 16:58:34]

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

[dewittj, 2014-09-17 16:58:48]

https://codereview.chromium.org/578883003/diff/1/chrome/browser/chrome_conten...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/578883003/diff/1/chrome/browser/chrome_conten...
chrome/browser/chrome_content_browser_client.cc:1912:
DesktopNotificationService* service =
So DNS needs a Profile* (as opposed to a BrowserContext) because of
permissions/content settings, right?

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

https://codereview.chromium.org/578883003/diff/1/chrome/browser/notifications...
chrome/browser/notifications/desktop_notification_service.cc:171: base::string16
DesktopNotificationService::DisplayNameForOrigin(
+dcheng who wrote GetExtensionsWithAPIPermissionForSecurityOrigin

[Peter Beverloo, 2014-09-17 17:06:06]

https://codereview.chromium.org/578883003/diff/1/chrome/browser/chrome_conten...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/578883003/diff/1/chrome/browser/chrome_conten...
chrome/browser/chrome_content_browser_client.cc:1912:
DesktopNotificationService* service =
On 2014/09/17 16:58:48, dewittj wrote:
> So DNS needs a Profile* (as opposed to a BrowserContext) because of
> permissions/content settings, right?

We use the preferences (Profile::GetPrefs()) and host content settings
(Profile::GetHostContentSettingsMap()). Chrome OS also uses the Profile to get
the User ID (GetUserIDFromProfile) to associate a
MessageCenterNotificationManager::ProfileNotification.

ExtensionWelcomeNotification also needs to be created using the profile_, but we
should be able to factor that out without too much trouble.

Everything else is happy with a BrowserContext.

[Jun Mukai, 2014-09-18 18:00:59]

I believe there are a few more ContentBrowserClient implementations you'll have
to modify:
https://code.google.com/p/chromium/codesearch#search/&q=public.*%5CbContentBr...

https://codereview.chromium.org/578883003/diff/1/chrome/browser/chrome_conten...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/578883003/diff/1/chrome/browser/chrome_conten...
chrome/browser/chrome_content_browser_client.cc:1915: params, delegate.Pass(),
cancel_callback);
doesn't this fit into the previous line?

[Peter Beverloo, 2014-10-03 14:39:30]

dcheng: ping re: question in DesktopNotificationService::DisplayNameForOrigin
(PS1).

https://codereview.chromium.org/578883003/diff/1/chrome/browser/chrome_conten...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/578883003/diff/1/chrome/browser/chrome_conten...
chrome/browser/chrome_content_browser_client.cc:1915: params, delegate.Pass(),
cancel_callback);
On 2014/09/18 18:00:59, Jun Mukai wrote:
> doesn't this fit into the previous line?

Done.

[dcheng, 2014-10-03 18:40:06]

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

https://codereview.chromium.org/578883003/diff/1/chrome/browser/notifications...
chrome/browser/notifications/desktop_notification_service.cc:171: base::string16
DesktopNotificationService::DisplayNameForOrigin(
On 2014/09/17 16:58:48, dewittj wrote:
> +dcheng who wrote GetExtensionsWithAPIPermissionForSecurityOrigin

It looks like I did this since it was suggested by aa@:
https://codereview.chromium.org/8312005#msg3

It looks like we originally could end up with apps in the wrong process somehow?
I'm not sure if this can still happen.

[Peter Beverloo, 2014-10-17 14:50:26]

peter@chromium.org changed reviewers:
+ aa@chromium.org, kalman@chromium.org

[Peter Beverloo, 2014-10-17 14:50:26]

OK, thanks. Aaron: any idea if the process Id check for extensions running in
the same process is still required?

I've also cc'ed kalman@ as he's likely to know this too.

[not at google - send to devlin, 2014-10-17 15:51:16]

> I've also cc'ed kalman@ as he's likely to know this too.

What is the code in question?

[Peter Beverloo, 2014-10-17 16:00:27]

On 2014/10/17 15:51:16, kalman wrote:
> > I've also cc'ed kalman@ as he's likely to know this too.
> 
> What is the code in question?

DesktopNotificationService::DisplayNameForOrigin in
desktop_notification_service.cc. Patch set 1 contains the discussion.

[not at google - send to devlin, 2014-10-17 16:12:27]

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

https://codereview.chromium.org/578883003/diff/1/chrome/browser/notifications...
chrome/browser/notifications/desktop_notification_service.cc:171: base::string16
DesktopNotificationService::DisplayNameForOrigin(
On 2014/10/03 18:40:06, dcheng wrote:
> On 2014/09/17 16:58:48, dewittj wrote:
> > +dcheng who wrote GetExtensionsWithAPIPermissionForSecurityOrigin
> 
> It looks like I did this since it was suggested by aa@:
> https://codereview.chromium.org/8312005#msg3
> 
> It looks like we originally could end up with apps in the wrong process
somehow?
> I'm not sure if this can still happen.

Extension pages can end up being hosted in other processes if they're in
iframes. Perhaps this code was to guard against that: say http://google.com has
an iframe to extension "bbb", and that iframe tries to show a notification. With
the old code that wouldn't pass because the process ID is for the web renderer
not the extension's renderer. With the new code it would pass because you just
check the "bbb". The reason for all of this is that we don't trust extension
renderers hosted in web processes.

[Peter Beverloo, 2014-10-24 13:20:19]

peter@chromium.org changed reviewers:
+ jochen@chromium.org, mkwst@chromium.org
- aa@chromium.org, dcheng@chromium.org

[Peter Beverloo, 2014-10-24 13:20:19]

Ok, thanks Ben. Then we leave the existing behavior intact. I've plumbed through
the render_process_id instead of the RenderViewHost* now.

dewittj: PTAL for c/b/notifications/
jochen: ContentBrowserClient changes (prototype change).
mkwst: FYI

[Peter Beverloo, 2014-10-24 13:21:41]

peter@chromium.org changed reviewers:
+ benm@chromium.org

[Peter Beverloo, 2014-10-24 13:21:42]

+benm@ for aw/ rs

[benm (inactive), 2014-10-24 13:29:33]

a_w lgtm

[Peter Beverloo, 2014-10-24 13:58:43]

peter@chromium.org changed reviewers:
+ jam@chromium.org
- jochen@chromium.org

[Peter Beverloo, 2014-10-24 13:58:43]

-jochen, +jam for small ContentBrowserClient API change.

[dewittj, 2014-10-24 20:17:37]

c/b/notifications lgtm

[Mike West, 2014-10-25 06:03:26]

c/s LGTM.

[Avi (use Gerrit), 2014-10-27 17:02:20]

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

[Avi (use Gerrit), 2014-10-27 17:02:21]

content/public LGTM

[Avi (use Gerrit), 2014-10-27 17:04:08]

On 2014/10/27 17:02:21, Avi wrote:
> content/public LGTM

content/browser LGTM 2

[Peter Beverloo, 2014-10-27 17:05:07]

The CQ bit was checked by peter@chromium.org

[commit-bot: I haz the power, 2014-10-27 17:05:29]

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

[commit-bot: I haz the power, 2014-10-27 17:59:30]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2014-10-27 18:00:14]

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