Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
220273002:
Use cross-platform base::UncheckedMalloc for skia (Closed)
|
Created:
6 years, 8 months ago by kbalazs Modified:
6 years, 8 months ago Reviewers:
Ken Russell (switch to Gerrit), reed2, bsalomon_chromium, bungeman-skia, Nico, bsalomon, mtklein CC:
chromium-reviews, erikwright+watch_chromium.org, Ken Russell (switch to Gerrit), Justin Novosad, willchan no longer on Chromium Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionUse cross-platform base::UncheckedMalloc for skia
UncheckedMalloc/UncheckedCalloc is now implemented for Linux as well so we can use it.
BUG=73751
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=261643
Patch Set 1 #Patch Set 2 : dont try to update comment as it just gets messier #
Total comments: 4
Patch Set 3 : fix typos #
Total comments: 2
Patch Set 4 : ignore_result #Patch Set 5 : fix ios build #
Messages
Total messages: 26 (0 generated)
Brian and Mike should probably review this too. https://codereview.chromium.org/220273002/diff/20001/skia/ext/SkMemory_new_ha... File skia/ext/SkMemory_new_handler.cpp (right): https://codereview.chromium.org/220273002/diff/20001/skia/ext/SkMemory_new_ha... skia/ext/SkMemory_new_handler.cpp:55: // doesn't not work as intended everywhere. "doesn't not work"? Perhaps "doesn't work"? https://codereview.chromium.org/220273002/diff/20001/skia/ext/SkMemory_new_ha... skia/ext/SkMemory_new_handler.cpp:86: // doesn't not work as intended everywhere. Same here
From my limited familiarity with Chromium's memory handling this seems ok but I'm definitely not an expert. Mike's out this week. I've added bungeman and mtklein in case they have an opinion. I'm happy to rubber stamp if not. https://codereview.chromium.org/220273002/diff/20001/skia/ext/SkMemory_new_ha... File skia/ext/SkMemory_new_handler.cpp (right): https://codereview.chromium.org/220273002/diff/20001/skia/ext/SkMemory_new_ha... skia/ext/SkMemory_new_handler.cpp:57: #defined(OS_MACOSX) || #defined(OS_ANDROID) Is this syntax correct, "#defined" rather than "defined"?
LGTM I think I touched this last. Nice to see the ability to catch malloc failures expanding. +1 to s/#defined/defined/
https://codereview.chromium.org/220273002/diff/20001/skia/ext/SkMemory_new_ha... File skia/ext/SkMemory_new_handler.cpp (right): https://codereview.chromium.org/220273002/diff/20001/skia/ext/SkMemory_new_ha... skia/ext/SkMemory_new_handler.cpp:57: #defined(OS_MACOSX) || #defined(OS_ANDROID) On 2014/04/01 12:12:51, bsalomon1 wrote: > Is this syntax correct, "#defined" rather than "defined"? Oops, it was a mistake. It built though so it seems like it's allowed :)
The CQ bit was checked by b.kelemen@samsung.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/b.kelemen@samsung.com/220273002/40001
The CQ bit was unchecked by commit-bot@chromium.org
No LGTM from a valid reviewer yet. Only full committers are accepted. Even if an LGTM may have been provided, it was from a non-committer or a lowly provisional committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
On 2014/04/01 20:12:50, I haz the power (commit-bot) wrote: > No LGTM from a valid reviewer yet. Only full committers are accepted. > Even if an LGTM may have been provided, it was from a non-committer or > a lowly provisional committer, _not_ a full super star committer. > See http://www.chromium.org/getting-involved/become-a-committer > Note that this has nothing to do with OWNERS files. Oops, I need an owner lgtm.
On 2014/04/01 20:58:00, kbalazs wrote: > On 2014/04/01 20:12:50, I haz the power (commit-bot) wrote: > > No LGTM from a valid reviewer yet. Only full committers are accepted. > > Even if an LGTM may have been provided, it was from a non-committer or > > a lowly provisional committer, _not_ a full super star committer. > > See http://www.chromium.org/getting-involved/become-a-committer > > Note that this has nothing to do with OWNERS files. > > Oops, I need an owner lgtm. I'm an owner of skia/ext. lgtm. But I think what you really need is a full committer lgtm, which I am not.
Looks mostly fine. https://codereview.chromium.org/220273002/diff/40001/skia/ext/SkMemory_new_ha... File skia/ext/SkMemory_new_handler.cpp (right): https://codereview.chromium.org/220273002/diff/40001/skia/ext/SkMemory_new_ha... skia/ext/SkMemory_new_handler.cpp:60: bool success ALLOW_UNUSED = base::UncheckedMalloc(size, &result); Why ALLOW_UNUSED?
https://codereview.chromium.org/220273002/diff/40001/skia/ext/SkMemory_new_ha... File skia/ext/SkMemory_new_handler.cpp (right): https://codereview.chromium.org/220273002/diff/40001/skia/ext/SkMemory_new_ha... skia/ext/SkMemory_new_handler.cpp:60: bool success ALLOW_UNUSED = base::UncheckedMalloc(size, &result); On 2014/04/01 21:05:02, Nico wrote: > Why ALLOW_UNUSED? (i.e. I think you want ignore_result() here. also below.)
The CQ bit was checked by b.kelemen@samsung.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/b.kelemen@samsung.com/220273002/60001
The CQ bit was unchecked by commit-bot@chromium.org
No LGTM from a valid reviewer yet. Only full committers are accepted. Even if an LGTM may have been provided, it was from a non-committer or a lowly provisional committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
patch set 4 lgtm
The CQ bit was checked by b.kelemen@samsung.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/b.kelemen@samsung.com/220273002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on ios_rel_device_ninja for step(s) compile http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=ios_rel_de...
On 2014/04/02 01:14:43, I haz the power (commit-bot) wrote: > Retried try job too often on ios_rel_device_ninja for step(s) compile > http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=ios_rel_de... changed defined(OS_MACOSX) check to (defined(OS_MACOSX) && !defined(OS_IOS)) because it seems like memory_mac.mm is only built on Mac. Please shout if you think this is wrong, otherwise I try to land if the trybots going to be green.
The CQ bit was checked by b.kelemen@samsung.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/b.kelemen@samsung.com/220273002/80001
Message was sent while issue was closed.
Change committed as 261643 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
