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

Issue 631833002: Move color chooser objects to the Oilpan heap. (Closed)

Created:
6 years, 2 months ago by sof
Modified:
6 years, 2 months ago
CC:
blink-reviews, dglazkov+blink, blink-reviews-html_chromium.org, gavinp+loader_chromium.org, Nate Chapin, mkwst+moarreviews_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/blink.git@master
Project:
blink
Visibility:
Public.

Description

Move color chooser objects to the Oilpan heap. Keep these on the heap so that their raw pointers to heap objects can be correctly traced. R=mkwst,haraken,tkent,keishi BUG=340522 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=183538

Patch Set 1 #

Total comments: 17

Patch Set 2 : Safer finalization #

Total comments: 1

Patch Set 3 : Make ColorChooserUIController::m_client protected, not private #

Total comments: 6

Patch Set 4 : assert adjustments + rebase #

Patch Set 5 : Keep ~ColorInputType empty #

Unified diffs Side-by-side diffs Delta from patch set Stats (+72 lines, -37 lines) Patch
M Source/core/html/forms/ColorChooser.h View 1 chunk +4 lines, -1 line 0 comments Download
M Source/core/html/forms/ColorChooserClient.h View 1 chunk +3 lines, -3 lines 0 comments Download
M Source/core/html/forms/ColorInputType.h View 1 2 3 2 chunks +3 lines, -1 line 0 comments Download
M Source/core/html/forms/ColorInputType.cpp View 2 3 4 1 chunk +7 lines, -1 line 0 comments Download
M Source/core/loader/EmptyClients.h View 1 2 3 1 chunk +1 line, -1 line 0 comments Download
M Source/core/loader/EmptyClients.cpp View 1 2 3 1 chunk +1 line, -1 line 0 comments Download
M Source/core/page/Chrome.h View 1 2 3 2 chunks +2 lines, -2 lines 0 comments Download
M Source/core/page/Chrome.cpp View 1 chunk +1 line, -1 line 0 comments Download
M Source/core/page/ChromeClient.h View 1 3 chunks +3 lines, -4 lines 0 comments Download
M Source/web/ChromeClientImpl.h View 1 2 3 2 chunks +3 lines, -3 lines 0 comments Download
M Source/web/ChromeClientImpl.cpp View 1 chunk +4 lines, -4 lines 0 comments Download
M Source/web/ColorChooserPopupUIController.h View 1 2 3 2 chunks +8 lines, -3 lines 0 comments Download
M Source/web/ColorChooserPopupUIController.cpp View 1 2 3 chunks +4 lines, -3 lines 0 comments Download
M Source/web/ColorChooserUIController.h View 1 2 3 3 chunks +14 lines, -5 lines 0 comments Download
M Source/web/ColorChooserUIController.cpp View 1 2 3 2 chunks +14 lines, -4 lines 0 comments Download

Messages

Total messages: 40 (8 generated)
sof
Please take a look. Removing yet another LocalFrame raw pointer reference (and a few other ...
6 years, 2 months ago (2014-10-06 17:46:21 UTC) #2
Mike West
https://codereview.chromium.org/631833002/diff/1/Source/core/html/forms/ColorInputType.cpp File Source/core/html/forms/ColorInputType.cpp (left): https://codereview.chromium.org/631833002/diff/1/Source/core/html/forms/ColorInputType.cpp#oldcode85 Source/core/html/forms/ColorInputType.cpp:85: endColorChooser(); Why is it safe to drop this call? ...
6 years, 2 months ago (2014-10-06 18:45:32 UTC) #4
sof
https://codereview.chromium.org/631833002/diff/1/Source/core/html/forms/ColorInputType.cpp File Source/core/html/forms/ColorInputType.cpp (left): https://codereview.chromium.org/631833002/diff/1/Source/core/html/forms/ColorInputType.cpp#oldcode85 Source/core/html/forms/ColorInputType.cpp:85: endColorChooser(); On 2014/10/06 18:45:32, Mike West wrote: > Why ...
6 years, 2 months ago (2014-10-06 19:19:29 UTC) #5
Mike West
core/ LGTM. https://codereview.chromium.org/631833002/diff/1/Source/core/html/forms/ColorInputType.cpp File Source/core/html/forms/ColorInputType.cpp (left): https://codereview.chromium.org/631833002/diff/1/Source/core/html/forms/ColorInputType.cpp#oldcode85 Source/core/html/forms/ColorInputType.cpp:85: endColorChooser(); On 2014/10/06 19:19:29, sof wrote: > ...
6 years, 2 months ago (2014-10-06 19:29:29 UTC) #6
Mike West
(web/ LGTM too, but you'll need an OWNER to sign off on it.)
6 years, 2 months ago (2014-10-06 19:29:53 UTC) #7
tkent
lgtm https://codereview.chromium.org/631833002/diff/1/Source/web/ColorChooserPopupUIController.cpp File Source/web/ColorChooserPopupUIController.cpp (right): https://codereview.chromium.org/631833002/diff/1/Source/web/ColorChooserPopupUIController.cpp#newcode58 Source/web/ColorChooserPopupUIController.cpp:58: endChooser(); endChooser() touches m_chooser and m_popup, and both ...
6 years, 2 months ago (2014-10-07 02:25:00 UTC) #9
haraken
LGTM https://codereview.chromium.org/631833002/diff/1/Source/web/ColorChooserPopupUIController.cpp File Source/web/ColorChooserPopupUIController.cpp (right): https://codereview.chromium.org/631833002/diff/1/Source/web/ColorChooserPopupUIController.cpp#newcode56 Source/web/ColorChooserPopupUIController.cpp:56: ColorChooserPopupUIController::~ColorChooserPopupUIController() Can we add ASSERT(!m_popup) ? https://codereview.chromium.org/631833002/diff/1/Source/web/ColorChooserPopupUIController.cpp#newcode58 Source/web/ColorChooserPopupUIController.cpp:58: ...
6 years, 2 months ago (2014-10-07 03:31:30 UTC) #11
sof
https://codereview.chromium.org/631833002/diff/1/Source/web/ColorChooserPopupUIController.cpp File Source/web/ColorChooserPopupUIController.cpp (right): https://codereview.chromium.org/631833002/diff/1/Source/web/ColorChooserPopupUIController.cpp#newcode56 Source/web/ColorChooserPopupUIController.cpp:56: ColorChooserPopupUIController::~ColorChooserPopupUIController() On 2014/10/07 03:31:29, haraken wrote: > > Can ...
6 years, 2 months ago (2014-10-07 06:35:20 UTC) #12
haraken
https://codereview.chromium.org/631833002/diff/1/Source/web/ColorChooserPopupUIController.cpp File Source/web/ColorChooserPopupUIController.cpp (right): https://codereview.chromium.org/631833002/diff/1/Source/web/ColorChooserPopupUIController.cpp#newcode56 Source/web/ColorChooserPopupUIController.cpp:56: ColorChooserPopupUIController::~ColorChooserPopupUIController() On 2014/10/07 06:35:19, sof wrote: > On 2014/10/07 ...
6 years, 2 months ago (2014-10-07 06:43:59 UTC) #13
sof
On 2014/10/07 06:43:59, haraken wrote: > https://codereview.chromium.org/631833002/diff/1/Source/web/ColorChooserPopupUIController.cpp > File Source/web/ColorChooserPopupUIController.cpp (right): > > https://codereview.chromium.org/631833002/diff/1/Source/web/ColorChooserPopupUIController.cpp#newcode56 > ...
6 years, 2 months ago (2014-10-07 06:59:56 UTC) #14
sof
On 2014/10/07 06:59:56, sof wrote: > On 2014/10/07 06:43:59, haraken wrote: > > > https://codereview.chromium.org/631833002/diff/1/Source/web/ColorChooserPopupUIController.cpp ...
6 years, 2 months ago (2014-10-07 07:48:23 UTC) #15
Mike West
Still LGTM.
6 years, 2 months ago (2014-10-07 07:53:18 UTC) #16
sof
https://codereview.chromium.org/631833002/diff/20001/Source/web/ColorChooserPopupUIController.h File Source/web/ColorChooserPopupUIController.h (right): https://codereview.chromium.org/631833002/diff/20001/Source/web/ColorChooserPopupUIController.h#newcode73 Source/web/ColorChooserPopupUIController.h:73: RawPtrWillBeMember<ColorChooserClient> m_client; Wait a sec, this is redundant - ...
6 years, 2 months ago (2014-10-07 07:59:22 UTC) #17
sof
On 2014/10/07 07:59:22, sof wrote: > https://codereview.chromium.org/631833002/diff/20001/Source/web/ColorChooserPopupUIController.h > File Source/web/ColorChooserPopupUIController.h (right): > > https://codereview.chromium.org/631833002/diff/20001/Source/web/ColorChooserPopupUIController.h#newcode73 > ...
6 years, 2 months ago (2014-10-07 08:09:25 UTC) #18
haraken
https://codereview.chromium.org/631833002/diff/40001/Source/core/html/forms/ColorInputType.cpp File Source/core/html/forms/ColorInputType.cpp (right): https://codereview.chromium.org/631833002/diff/40001/Source/core/html/forms/ColorInputType.cpp#newcode87 Source/core/html/forms/ColorInputType.cpp:87: // its ColorChooserClient when doing so (Oilpan friendly finalization.) ...
6 years, 2 months ago (2014-10-07 08:27:36 UTC) #19
sof
https://codereview.chromium.org/631833002/diff/40001/Source/core/html/forms/ColorInputType.cpp File Source/core/html/forms/ColorInputType.cpp (right): https://codereview.chromium.org/631833002/diff/40001/Source/core/html/forms/ColorInputType.cpp#newcode87 Source/core/html/forms/ColorInputType.cpp:87: // its ColorChooserClient when doing so (Oilpan friendly finalization.) ...
6 years, 2 months ago (2014-10-07 08:31:45 UTC) #20
haraken
https://codereview.chromium.org/631833002/diff/40001/Source/core/html/forms/ColorInputType.cpp File Source/core/html/forms/ColorInputType.cpp (right): https://codereview.chromium.org/631833002/diff/40001/Source/core/html/forms/ColorInputType.cpp#newcode87 Source/core/html/forms/ColorInputType.cpp:87: // its ColorChooserClient when doing so (Oilpan friendly finalization.) ...
6 years, 2 months ago (2014-10-07 08:49:27 UTC) #21
sof
Can ~ColorInputType() acceptably not call didEndChooser()? https://codereview.chromium.org/631833002/diff/40001/Source/web/ColorChooserUIController.cpp File Source/web/ColorChooserUIController.cpp (right): https://codereview.chromium.org/631833002/diff/40001/Source/web/ColorChooserUIController.cpp#newcode90 Source/web/ColorChooserUIController.cpp:90: ASSERT(m_client); On 2014/10/07 ...
6 years, 2 months ago (2014-10-08 10:26:35 UTC) #22
haraken
> Can ~ColorInputType() acceptably not call didEndChooser()? I noticed that ~ColorInputType() cannot call didEndChooser() because ...
6 years, 2 months ago (2014-10-08 11:42:18 UTC) #23
sof
On 2014/10/08 11:42:18, haraken wrote: > > Can ~ColorInputType() acceptably not call didEndChooser()? > > ...
6 years, 2 months ago (2014-10-08 12:16:48 UTC) #24
haraken
On 2014/10/08 12:16:48, sof wrote: > On 2014/10/08 11:42:18, haraken wrote: > > > Can ...
6 years, 2 months ago (2014-10-08 12:47:42 UTC) #25
sof
+keishi
6 years, 2 months ago (2014-10-08 12:52:44 UTC) #27
sof
On 2014/10/08 12:47:42, haraken wrote: > On 2014/10/08 12:16:48, sof wrote: > > On 2014/10/08 ...
6 years, 2 months ago (2014-10-08 12:53:21 UTC) #28
haraken
LGTM, but keishi@ should confirm.
6 years, 2 months ago (2014-10-08 13:09:22 UTC) #29
sof
On 2014/10/08 13:09:22, haraken wrote: > LGTM, but keishi@ should confirm. Checking the other InputTypes, ...
6 years, 2 months ago (2014-10-08 14:40:39 UTC) #30
sof
keishi: looks acceptable?
6 years, 2 months ago (2014-10-10 06:03:38 UTC) #31
keishi
On 2014/10/10 06:03:38, sof wrote: > keishi: looks acceptable? Sorry, yeah this looks okay. LGTM
6 years, 2 months ago (2014-10-10 06:32:38 UTC) #32
sof
On 2014/10/10 06:32:38, keishi wrote: > On 2014/10/10 06:03:38, sof wrote: > > keishi: looks ...
6 years, 2 months ago (2014-10-10 06:41:45 UTC) #33
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/631833002/80001
6 years, 2 months ago (2014-10-10 06:42:53 UTC) #35
commit-bot: I haz the power
Try jobs failed on following builders: win_blink_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/31120)
6 years, 2 months ago (2014-10-10 08:31:05 UTC) #37
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/631833002/80001
6 years, 2 months ago (2014-10-10 09:43:35 UTC) #39
commit-bot: I haz the power
6 years, 2 months ago (2014-10-10 12:20:06 UTC) #40
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as 183538

Powered by Google App Engine
This is Rietveld 408576698