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

Issue 190183003: Oilpan: move ImageBitmap to the oilpan heap. (Closed)

Created:
6 years, 9 months ago by sof
Modified:
6 years, 9 months ago
CC:
blink-reviews, arv+blink, Inactive, watchdog-blink-watchlist_google.com
Base URL:
https://chromium.googlesource.com/chromium/blink.git@master
Visibility:
Public.

Description

Patch Set 1 #

Patch Set 2 : Timely removal of image resources from test memory cache #

Patch Set 3 : Have ImageLoader keep a persistent set of ImageLoaderClients #

Total comments: 19

Patch Set 4 : Streamlining #

Patch Set 5 : Make ImageLoader hold weak references to its clients #

Patch Set 6 : Rebase #

Patch Set 7 : Switch to using RefPtrWillBeMember<> for stack allocated class. #

Patch Set 8 : Control image resource lifetimes in tests using explicit GCs #

Total comments: 2
Unified diffs Side-by-side diffs Delta from patch set Stats (+67 lines, -38 lines) Patch
M Source/core/frame/ImageBitmap.h View 1 2 3 4 5 6 7 3 chunks +11 lines, -8 lines 0 comments Download
M Source/core/frame/ImageBitmap.cpp View 1 2 3 4 5 6 7 2 chunks +16 lines, -12 lines 0 comments Download
M Source/core/frame/ImageBitmap.idl View 1 chunk +1 line, -0 lines 0 comments Download
M Source/core/frame/ImageBitmapTest.cpp View 1 2 3 4 5 6 7 8 chunks +28 lines, -12 lines 0 comments Download
M Source/core/loader/ImageLoader.h View 1 2 3 4 2 chunks +7 lines, -2 lines 2 comments Download
M Source/core/loader/ImageLoader.cpp View 1 2 3 4 5 1 chunk +2 lines, -2 lines 0 comments Download
M Source/modules/imagebitmap/ImageBitmapFactories.cpp View 1 2 3 4 5 2 chunks +2 lines, -2 lines 0 comments Download

Messages

Total messages: 36 (0 generated)
sof
Please take a look.
6 years, 9 months ago (2014-03-08 15:07:20 UTC) #1
Erik Corry
ImageBitmap inherits from ImageLoaderClient, and there is a weak pointer to the ImageLoaderClient, which is ...
6 years, 9 months ago (2014-03-10 10:20:24 UTC) #2
sof
On 2014/03/10 10:20:24, Erik Corry wrote: > ImageBitmap inherits from ImageLoaderClient, and there is a ...
6 years, 9 months ago (2014-03-10 10:49:55 UTC) #3
sof
On 2014/03/10 10:49:55, sof wrote: > On 2014/03/10 10:20:24, Erik Corry wrote: > > ImageBitmap ...
6 years, 9 months ago (2014-03-10 13:43:30 UTC) #4
haraken
https://codereview.chromium.org/190183003/diff/40001/Source/core/frame/ImageBitmap.h File Source/core/frame/ImageBitmap.h (right): https://codereview.chromium.org/190183003/diff/40001/Source/core/frame/ImageBitmap.h#newcode44 Source/core/frame/ImageBitmap.h:44: void detach(); detach => dispose. attach/detach is normally used ...
6 years, 9 months ago (2014-03-10 15:25:03 UTC) #5
sof
https://codereview.chromium.org/190183003/diff/40001/Source/core/frame/ImageBitmap.h File Source/core/frame/ImageBitmap.h (right): https://codereview.chromium.org/190183003/diff/40001/Source/core/frame/ImageBitmap.h#newcode46 Source/core/frame/ImageBitmap.h:46: virtual void trace(Visitor*); On 2014/03/10 15:25:03, haraken wrote: > ...
6 years, 9 months ago (2014-03-10 15:55:57 UTC) #6
haraken
https://codereview.chromium.org/190183003/diff/40001/Source/core/frame/ImageBitmapTest.cpp File Source/core/frame/ImageBitmapTest.cpp (right): https://codereview.chromium.org/190183003/diff/40001/Source/core/frame/ImageBitmapTest.cpp#newcode97 Source/core/frame/ImageBitmapTest.cpp:97: class AutoImageBitmap { On 2014/03/10 15:55:57, sof wrote: > ...
6 years, 9 months ago (2014-03-10 16:04:54 UTC) #7
sof
https://codereview.chromium.org/190183003/diff/40001/Source/core/frame/ImageBitmap.h File Source/core/frame/ImageBitmap.h (right): https://codereview.chromium.org/190183003/diff/40001/Source/core/frame/ImageBitmap.h#newcode44 Source/core/frame/ImageBitmap.h:44: void detach(); On 2014/03/10 15:25:03, haraken wrote: > > ...
6 years, 9 months ago (2014-03-10 21:07:20 UTC) #8
Erik Corry
I think this change now makes the ImageLoader have a strong reference to the ImageBitmaps, ...
6 years, 9 months ago (2014-03-10 21:18:56 UTC) #9
haraken
On 2014/03/10 21:18:56, Erik Corry wrote: > I think this change now makes the ImageLoader ...
6 years, 9 months ago (2014-03-11 02:46:56 UTC) #10
sof
On 2014/03/10 21:18:56, Erik Corry wrote: > I think this change now makes the ImageLoader ...
6 years, 9 months ago (2014-03-11 07:26:50 UTC) #11
sof
On 2014/03/11 02:46:56, haraken wrote: > On 2014/03/10 21:18:56, Erik Corry wrote: > > I ...
6 years, 9 months ago (2014-03-11 07:55:38 UTC) #12
haraken
> > Another viewpoint is that all Members in objects should be visited via trace() ...
6 years, 9 months ago (2014-03-11 07:56:38 UTC) #13
Erik Corry
We discussed this and the decision was that it's OK (even preferred) to have Member<T> ...
6 years, 9 months ago (2014-03-11 09:14:07 UTC) #14
sof
On 2014/03/11 09:14:07, Erik Corry wrote: > We discussed this and the decision was that ...
6 years, 9 months ago (2014-03-11 09:26:18 UTC) #15
sof
https://codereview.chromium.org/190183003/diff/40001/Source/core/frame/ImageBitmapTest.cpp File Source/core/frame/ImageBitmapTest.cpp (right): https://codereview.chromium.org/190183003/diff/40001/Source/core/frame/ImageBitmapTest.cpp#newcode97 Source/core/frame/ImageBitmapTest.cpp:97: class AutoImageBitmap { On 2014/03/10 21:07:21, sof wrote: > ...
6 years, 9 months ago (2014-03-12 10:02:25 UTC) #16
Erik Corry
https://codereview.chromium.org/190183003/diff/40001/Source/core/frame/ImageBitmapTest.cpp File Source/core/frame/ImageBitmapTest.cpp (right): https://codereview.chromium.org/190183003/diff/40001/Source/core/frame/ImageBitmapTest.cpp#newcode97 Source/core/frame/ImageBitmapTest.cpp:97: class AutoImageBitmap { On 2014/03/12 10:02:25, sof wrote: > ...
6 years, 9 months ago (2014-03-12 10:06:14 UTC) #17
haraken
https://codereview.chromium.org/190183003/diff/40001/Source/core/frame/ImageBitmapTest.cpp File Source/core/frame/ImageBitmapTest.cpp (right): https://codereview.chromium.org/190183003/diff/40001/Source/core/frame/ImageBitmapTest.cpp#newcode97 Source/core/frame/ImageBitmapTest.cpp:97: class AutoImageBitmap { On 2014/03/12 10:06:15, Erik Corry wrote: ...
6 years, 9 months ago (2014-03-12 10:34:08 UTC) #18
sof
On 2014/03/12 10:34:08, haraken wrote: > https://codereview.chromium.org/190183003/diff/40001/Source/core/frame/ImageBitmapTest.cpp > File Source/core/frame/ImageBitmapTest.cpp (right): > > https://codereview.chromium.org/190183003/diff/40001/Source/core/frame/ImageBitmapTest.cpp#newcode97 > ...
6 years, 9 months ago (2014-03-12 14:14:51 UTC) #19
Erik Corry
The HeapHashSet is not heap allocated, but as soon as it is not empty it ...
6 years, 9 months ago (2014-03-12 14:29:07 UTC) #20
sof
On 2014/03/12 14:29:07, Erik Corry wrote: > The HeapHashSet is not heap allocated, but as ...
6 years, 9 months ago (2014-03-12 14:54:31 UTC) #21
Erik Corry
OK that sounds like a wrong assert. It should be OK to register the hash ...
6 years, 9 months ago (2014-03-12 14:59:44 UTC) #22
Erik Corry
Ah, I see it's not just an assert: We need to change the API a ...
6 years, 9 months ago (2014-03-12 15:05:55 UTC) #23
sof
On 2014/03/12 15:05:55, Erik Corry wrote: > Ah, I see it's not just an assert: ...
6 years, 9 months ago (2014-03-12 15:13:45 UTC) #24
Erik Corry
This should help: https://codereview.chromium.org/197343003
6 years, 9 months ago (2014-03-12 15:34:00 UTC) #25
sof
On 2014/03/12 15:34:00, Erik Corry wrote: > This should help: https://codereview.chromium.org/197343003 Confirmedly does; awesome, thanks ...
6 years, 9 months ago (2014-03-12 16:03:48 UTC) #26
sof
On 2014/03/12 15:13:45, sof wrote: ... > > (The use of collectGarbage() isn't ideal for ...
6 years, 9 months ago (2014-03-12 21:53:56 UTC) #27
haraken
LGTM with a comment. https://codereview.chromium.org/190183003/diff/120001/Source/core/loader/ImageLoader.h File Source/core/loader/ImageLoader.h (right): https://codereview.chromium.org/190183003/diff/120001/Source/core/loader/ImageLoader.h#newcode112 Source/core/loader/ImageLoader.h:112: typedef WillBePersistentHeapHashSet<RawPtrWillBeWeakMember<ImageLoaderClient> > ImageLoaderClientSet; Can't ...
6 years, 9 months ago (2014-03-13 01:22:28 UTC) #28
sof
https://codereview.chromium.org/190183003/diff/120001/Source/core/loader/ImageLoader.h File Source/core/loader/ImageLoader.h (right): https://codereview.chromium.org/190183003/diff/120001/Source/core/loader/ImageLoader.h#newcode112 Source/core/loader/ImageLoader.h:112: typedef WillBePersistentHeapHashSet<RawPtrWillBeWeakMember<ImageLoaderClient> > ImageLoaderClientSet; On 2014/03/13 01:22:28, haraken wrote: ...
6 years, 9 months ago (2014-03-13 06:14:36 UTC) #29
sof
https://codereview.chromium.org/197343003/ now landed; sending this one along.
6 years, 9 months ago (2014-03-13 14:57:22 UTC) #30
sof
The CQ bit was checked by sigbjornf@opera.com
6 years, 9 months ago (2014-03-13 14:57:55 UTC) #31
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sigbjornf@opera.com/190183003/120001
6 years, 9 months ago (2014-03-13 14:57:57 UTC) #32
sof
The CQ bit was unchecked by sigbjornf@opera.com
6 years, 9 months ago (2014-03-13 19:23:37 UTC) #33
sof
The CQ bit was checked by sigbjornf@opera.com
6 years, 9 months ago (2014-03-13 19:24:04 UTC) #34
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sigbjornf@opera.com/190183003/120001
6 years, 9 months ago (2014-03-13 19:24:15 UTC) #35
commit-bot: I haz the power
6 years, 9 months ago (2014-03-13 21:53:54 UTC) #36
Message was sent while issue was closed.
Change committed as 169153

Powered by Google App Engine
This is Rietveld 408576698