Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(119)

Issue 469313002: Revert 289532 "Route newly created notifications to notification..." (Closed)

Created:
6 years, 4 months ago by bartfab (slow)
Modified:
6 years, 4 months ago
Reviewers:
liyanhou
CC:
chromium-reviews
Visibility:
Public.

Description

Revert 289532 "Route newly created notifications to notification..." This CL introduces a memory leak. Line 126 of notification_conversion_helper.cc contains a new[] that has no corresponding delete[]: unsigned char* bitmap_data(new unsigned char[pixel_count * BYTES_PER_PIXEL]); > Route newly created notifications to notification provider API. > > Whenever a new notification is created, route it to an > extension/app that has notification provider permission. > Currently, if there's an app with notificationProvider > permission, notifications will go to both message center > and that app. In the future, when additional sections in > Chrome Settings is implemented, this will send notifications > to only one party based on the user's selection. > > BUG= > > Review URL: https://codereview.chromium.org/441753002 TBR=liyanhou@chromium.org Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=289544

Patch Set 1 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+163 lines, -529 lines) Patch
M trunk/src/chrome/browser/extensions/api/notification_provider/notification_provider_apitest.cc View 1 chunk +43 lines, -1 line 0 comments Download
M trunk/src/chrome/browser/extensions/api/notifications/notifications_api.cc View 9 chunks +91 lines, -37 lines 0 comments Download
M trunk/src/chrome/browser/notifications/message_center_notification_manager.h View 3 chunks +2 lines, -9 lines 0 comments Download
M trunk/src/chrome/browser/notifications/message_center_notification_manager.cc View 5 chunks +13 lines, -68 lines 0 comments Download
D trunk/src/chrome/browser/notifications/notification_conversion_helper.h View 1 chunk +0 lines, -54 lines 0 comments Download
D trunk/src/chrome/browser/notifications/notification_conversion_helper.cc View 1 chunk +0 lines, -211 lines 0 comments Download
D trunk/src/chrome/browser/notifications/notification_conversion_helper_unittest.cc View 1 chunk +0 lines, -126 lines 0 comments Download
M trunk/src/chrome/chrome_browser.gypi View 1 chunk +0 lines, -2 lines 0 comments Download
M trunk/src/chrome/chrome_tests_unit.gypi View 1 chunk +0 lines, -1 line 0 comments Download
D trunk/src/chrome/test/data/extensions/api_test/notification_provider/events/icon.png View Binary file 0 comments Download
M trunk/src/chrome/test/data/extensions/api_test/notification_provider/events/manifest.json View 1 chunk +1 line, -1 line 0 comments Download
M trunk/src/chrome/test/data/extensions/api_test/notification_provider/events/test.js View 1 chunk +13 lines, -19 lines 0 comments Download

Messages

Total messages: 3 (0 generated)
bartfab (slow)
6 years, 4 months ago (2014-08-14 13:57:54 UTC) #1
bartfab (slow)
Committed patchset #1 manually as r289544 (tree was closed).
6 years, 4 months ago (2014-08-14 13:58:09 UTC) #2
liyanhou
6 years, 4 months ago (2014-08-14 21:30:02 UTC) #3
Message was sent while issue was closed.
On 2014/08/14 13:58:09, bartfab wrote:
> Committed patchset #1 manually as r289544 (tree was closed).

lgtm

Powered by Google App Engine
This is Rietveld 408576698