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

Issues Search
    My Issues | Starred     Open | Closed | All

Issue 469403002: Fix memory leak of Route newly created notifications to notification provider API (Closed)

Created:
6 years, 4 months ago by liyanhou
Modified:
6 years, 4 months ago
CC:
chromium-reviews, chromium-apps-reviews_chromium.org, extensions-reviews_chromium.org
Project:
chromium
Visibility:
Public.

Description

Fix memory leak in NotificationConversionHelper (CL 441753002) and re-upload the part routing newly created notifications to notification provider API. CL 441753002 was reverted. The only difference between this CL and CL 441753002 is: starting from line 126 of notification_conversion_helper.cc, changed a raw pointer (unsigned char* bitmap_data) to a scoped pointer (scoped_ptr<unsigned char[]> bitmap_data). BUG=403759 BUG=403775 Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=289776

Patch Set 1 #

Total comments: 2

Patch Set 2 : address comment #

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

Messages

Total messages: 10 (0 generated)
liyanhou
Fixed memory leak of previous CL Route newly created notifications to notification provider API. Could ...
6 years, 4 months ago (2014-08-14 20:07:17 UTC) #1
Pete Williamson
lgtm
6 years, 4 months ago (2014-08-14 20:12:40 UTC) #2
dewittj
lgtm please beware of operator new in the future... https://codereview.chromium.org/469403002/diff/1/chrome/browser/notifications/notification_conversion_helper.cc File chrome/browser/notifications/notification_conversion_helper.cc (right): https://codereview.chromium.org/469403002/diff/1/chrome/browser/notifications/notification_conversion_helper.cc#newcode126 chrome/browser/notifications/notification_conversion_helper.cc:126: ...
6 years, 4 months ago (2014-08-14 20:20:21 UTC) #3
liyanhou
The CQ bit was checked by liyanhou@chromium.org
6 years, 4 months ago (2014-08-14 20:21:03 UTC) #4
liyanhou
The CQ bit was unchecked by liyanhou@chromium.org
6 years, 4 months ago (2014-08-14 20:21:07 UTC) #5
liyanhou
On 2014/08/14 20:20:21, dewittj wrote: > lgtm > > please beware of operator new in ...
6 years, 4 months ago (2014-08-14 20:28:49 UTC) #6
liyanhou
https://codereview.chromium.org/469403002/diff/1/chrome/browser/notifications/notification_conversion_helper.cc File chrome/browser/notifications/notification_conversion_helper.cc (right): https://codereview.chromium.org/469403002/diff/1/chrome/browser/notifications/notification_conversion_helper.cc#newcode126 chrome/browser/notifications/notification_conversion_helper.cc:126: scoped_ptr<unsigned char[]> bitmap_data( On 2014/08/14 20:20:21, dewittj wrote: > ...
6 years, 4 months ago (2014-08-14 20:29:04 UTC) #7
liyanhou
The CQ bit was checked by liyanhou@chromium.org
6 years, 4 months ago (2014-08-14 20:29:09 UTC) #8
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/liyanhou@chromium.org/469403002/20001
6 years, 4 months ago (2014-08-14 20:31:03 UTC) #9
commit-bot: I haz the power
6 years, 4 months ago (2014-08-15 04:17:59 UTC) #10
Message was sent while issue was closed. 
Committed patchset #2 (20001) as 289776

Powered by Google App Engine
This is Rietveld 408576698