Only allows extensions to create HTML notifications.

Only allows extensions to create HTML notifications.

createHTMLNotification has been removed from the W3C Notifications spec. We do not want it to be exposed for web pages. However, since quite a few extensions are still using it, we would still keep it to be exposed for extensions.

BUG=98061
TEST=existing tests

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=149054

[Mihai Parparita -not on Chrome, 2012-07-26 17:27:12]

http://codereview.chromium.org/10808113/diff/1/chrome/renderer/chrome_render_...
File chrome/renderer/chrome_render_view_observer.cc (right):

http://codereview.chromium.org/10808113/diff/1/chrome/renderer/chrome_render_...
chrome/renderer/chrome_render_view_observer.cc:522: bool
ChromeRenderViewObserver::allowHTMLNotifications(
Can you add a test for this? It should be something similar to the test added
for allowWebCompontents (see shadow_dom_apitest.cc and api_test/shadow_dom/ in
http://crrev.com/145229)

[jianli, 2012-07-26 21:27:46]

http://codereview.chromium.org/10808113/diff/1/chrome/renderer/chrome_render_...
File chrome/renderer/chrome_render_view_observer.cc (right):

http://codereview.chromium.org/10808113/diff/1/chrome/renderer/chrome_render_...
chrome/renderer/chrome_render_view_observer.cc:522: bool
ChromeRenderViewObserver::allowHTMLNotifications(
On 2012/07/26 17:27:12, Mihai Parparita wrote:
> Can you add a test for this? It should be something similar to the test added
> for allowWebCompontents (see shadow_dom_apitest.cc and api_test/shadow_dom/ in
> http://crrev.com/145229)

I've fixed and re-enabled existing notification API tests to also cover this.

[Mihai Parparita -not on Chrome, 2012-07-26 23:59:04]

http://codereview.chromium.org/10808113/diff/5001/chrome/test/data/extensions...
File
chrome/test/data/extensions/api_test/notifications/has_permission_manifest/background.js
(left):

http://codereview.chromium.org/10808113/diff/5001/chrome/test/data/extensions...
chrome/test/data/extensions/api_test/notifications/has_permission_manifest/background.js:29:
if (tab.url == chromeExtensionsUrl) {
Why remove this comparison? Is it just a matter of using
chrome://chrome/extensions/ (the new URL for the settings page)?

http://codereview.chromium.org/10808113/diff/5001/chrome/test/data/extensions...
File
chrome/test/data/extensions/api_test/notifications/has_permission_prefs/background.js
(right):

http://codereview.chromium.org/10808113/diff/5001/chrome/test/data/extensions...
chrome/test/data/extensions/api_test/notifications/has_permission_prefs/background.js:35:
showHTMLNotification();
You might as well inline the test functions.

[Mihai Parparita -not on Chrome, 2012-07-26 23:59:38]

Also, should BUG=98061 be added to the CL?

Mihai

On Thu, Jul 26, 2012 at 2:27 PM, <jianli@chromium.org> wrote:

>
> http://codereview.chromium.**org/10808113/diff/1/chrome/**
>
renderer/chrome_render_view_**observer.cc<http://codereview.chromium.org/10808113/diff/1/chrome/renderer/chrome_render_view_observer.cc>
> File chrome/renderer/chrome_render_**view_observer.cc (right):
>
> http://codereview.chromium.**org/10808113/diff/1/chrome/**
>
renderer/chrome_render_view_**observer.cc#newcode522<http://codereview.chromium.org/10808113/diff/1/chrome/renderer/chrome_render_view_observer.cc#newcode522>
> chrome/renderer/chrome_render_**view_observer.cc:522: bool
> ChromeRenderViewObserver::**allowHTMLNotifications(
> On 2012/07/26 17:27:12, Mihai Parparita wrote:
>
>> Can you add a test for this? It should be something similar to the
>>
> test added
>
>> for allowWebCompontents (see shadow_dom_apitest.cc and
>>
> api_test/shadow_dom/ in
>
>> http://crrev.com/145229)
>>
>
> I've fixed and re-enabled existing notification API tests to also cover
> this.
>
>
http://codereview.chromium.**org/10808113/<http://codereview.chromium.org/108...
>

[jianli, 2012-07-27 00:34:10]

BUG=98061 is added to the CL.

http://codereview.chromium.org/10808113/diff/5001/chrome/test/data/extensions...
File
chrome/test/data/extensions/api_test/notifications/has_permission_manifest/background.js
(left):

http://codereview.chromium.org/10808113/diff/5001/chrome/test/data/extensions...
chrome/test/data/extensions/api_test/notifications/has_permission_manifest/background.js:29:
if (tab.url == chromeExtensionsUrl) {
On 2012/07/26 23:59:05, Mihai Parparita wrote:
> Why remove this comparison? Is it just a matter of using
> chrome://chrome/extensions/ (the new URL for the settings page)?

Yes, it is about the new settings URL. I think we do not care about the current
URL and rather leave it out in case that the settings URL gets changed again and
this test still works.

http://codereview.chromium.org/10808113/diff/5001/chrome/test/data/extensions...
File
chrome/test/data/extensions/api_test/notifications/has_permission_prefs/background.js
(right):

http://codereview.chromium.org/10808113/diff/5001/chrome/test/data/extensions...
chrome/test/data/extensions/api_test/notifications/has_permission_prefs/background.js:35:
showHTMLNotification();
On 2012/07/26 23:59:05, Mihai Parparita wrote:
> You might as well inline the test functions.

Done.

[Mihai Parparita -not on Chrome, 2012-07-27 00:43:22]

LGTM

[commit-bot: I haz the power, 2012-07-27 00:50:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jianli@chromium.org/10808113/6017

[commit-bot: I haz the power, 2012-07-27 00:50:55]

Presubmit check for 10808113-6017 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome

[jianli, 2012-07-27 00:55:30]

Sky, can you approve it as an OWNER? Thanks.

[jianli, 2012-07-30 18:29:26]

John, could you please approve this patch as an owner? Thanks.

[jam, 2012-07-30 20:19:45]

I'm heavily overloaded by reviews for content, so unless it's for code that I
wrote, please load balance by picking other reviewers in
http://src.chromium.org/viewvc/chrome/trunk/src/chrome/OWNERS?revision=148406...

I suggest one of 
jhawkins@chromium.org
thakis@chromium.org
thestig@chromium.org

[jam, 2012-07-30 20:33:02]

lgtm

[commit-bot: I haz the power, 2012-07-30 20:40:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jianli@chromium.org/10808113/6017

[commit-bot: I haz the power, 2012-07-30 22:24:36]

Change committed as 149054