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

Issue 1985033003: [DevTools] Use registerWeakMembers in NetworkResourcesData. (Closed)

Created:
4 years, 7 months ago by dgozman
Modified:
4 years, 7 months ago
CC:
chromium-reviews, tyoshino+watch_chromium.org, caseq+blink_chromium.org, lushnikov+blink_chromium.org, pfeldman+blink_chromium.org, apavlov+blink_chromium.org, gavinp+loader_chromium.org, devtools-reviews_chromium.org, blink-reviews, loading-reviews+fetch_chromium.org, sergeyv+blink_chromium.org, kozyatinskiy+blink_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

[DevTools] Use registerWeakMembers in NetworkResourcesData. We have custom plumbing from pre-finalizer via InspectorInstrumentation::willDestroyResource, which is no better than intended for this purpose registerWeakMembers call. BUG=603462 Committed: https://crrev.com/a25ff64ce15df2a38c3bf3f92f3272080e5640f5 Cr-Commit-Position: refs/heads/master@{#394567}

Patch Set 1 #

Total comments: 2

Patch Set 2 : removed instrumentation from deps #

Patch Set 3 : rebased, removed more code #

Unified diffs Side-by-side diffs Delta from patch set Stats (+35 lines, -80 lines) Patch
M third_party/WebKit/Source/core/fetch/DEPS View 1 1 chunk +0 lines, -1 line 0 comments Download
M third_party/WebKit/Source/core/fetch/Resource.cpp View 1 3 chunks +1 line, -2 lines 0 comments Download
M third_party/WebKit/Source/core/inspector/InspectorInstrumentation.cpp View 1 2 3 chunks +0 lines, -34 lines 0 comments Download
M third_party/WebKit/Source/core/inspector/InspectorInstrumentationCustomInl.h View 1 2 1 chunk +0 lines, -1 line 0 comments Download
M third_party/WebKit/Source/core/inspector/InspectorResourceAgent.cpp View 1 chunk +0 lines, -20 lines 0 comments Download
M third_party/WebKit/Source/core/inspector/NetworkResourcesData.h View 3 chunks +3 lines, -2 lines 0 comments Download
M third_party/WebKit/Source/core/inspector/NetworkResourcesData.cpp View 6 chunks +31 lines, -18 lines 0 comments Download
M third_party/WebKit/Source/web/WebDevToolsAgentImpl.cpp View 1 2 2 chunks +0 lines, -2 lines 0 comments Download

Messages

Total messages: 24 (12 generated)
dgozman
Could you please take a look?
4 years, 7 months ago (2016-05-17 18:08:33 UTC) #3
Nate Chapin
Quick driveby comment... https://codereview.chromium.org/1985033003/diff/1/third_party/WebKit/Source/core/fetch/Resource.cpp File third_party/WebKit/Source/core/fetch/Resource.cpp (left): https://codereview.chromium.org/1985033003/diff/1/third_party/WebKit/Source/core/fetch/Resource.cpp#oldcode330 third_party/WebKit/Source/core/fetch/Resource.cpp:330: InspectorInstrumentation::willDestroyResource(this); This is the last usage ...
4 years, 7 months ago (2016-05-17 18:23:36 UTC) #5
caseq
lgtm
4 years, 7 months ago (2016-05-17 18:24:19 UTC) #6
dgozman
https://codereview.chromium.org/1985033003/diff/1/third_party/WebKit/Source/core/fetch/Resource.cpp File third_party/WebKit/Source/core/fetch/Resource.cpp (left): https://codereview.chromium.org/1985033003/diff/1/third_party/WebKit/Source/core/fetch/Resource.cpp#oldcode330 third_party/WebKit/Source/core/fetch/Resource.cpp:330: InspectorInstrumentation::willDestroyResource(this); On 2016/05/17 18:23:36, Nate Chapin wrote: > This ...
4 years, 7 months ago (2016-05-17 18:34:26 UTC) #7
Nate Chapin
On 2016/05/17 18:34:26, dgozman wrote: > https://codereview.chromium.org/1985033003/diff/1/third_party/WebKit/Source/core/fetch/Resource.cpp > File third_party/WebKit/Source/core/fetch/Resource.cpp (left): > > https://codereview.chromium.org/1985033003/diff/1/third_party/WebKit/Source/core/fetch/Resource.cpp#oldcode330 > ...
4 years, 7 months ago (2016-05-17 18:40:47 UTC) #8
hiroshige
lgtm, thanks! Could you add BUG=603462 to CL description because this is related to https://codereview.chromium.org/1807323002/?
4 years, 7 months ago (2016-05-18 05:06:22 UTC) #9
dgozman
On 2016/05/18 05:06:22, hiroshige wrote: > lgtm, thanks! > > Could you add BUG=603462 to ...
4 years, 7 months ago (2016-05-18 05:43:52 UTC) #11
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1985033003/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1985033003/20001
4 years, 7 months ago (2016-05-18 05:45:00 UTC) #15
commit-bot: I haz the power
Try jobs failed on following builders: ios-device on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds/7428) ios-device-gn on tryserver.chromium.mac (JOB_FAILED, ...
4 years, 7 months ago (2016-05-18 05:47:12 UTC) #17
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1985033003/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1985033003/40001
4 years, 7 months ago (2016-05-18 20:36:57 UTC) #20
commit-bot: I haz the power
Committed patchset #3 (id:40001)
4 years, 7 months ago (2016-05-18 22:03:24 UTC) #22
commit-bot: I haz the power
4 years, 7 months ago (2016-05-18 22:05:02 UTC) #24
Message was sent while issue was closed.
Patchset 3 (id:??) landed as
https://crrev.com/a25ff64ce15df2a38c3bf3f92f3272080e5640f5
Cr-Commit-Position: refs/heads/master@{#394567}

Powered by Google App Engine
This is Rietveld 408576698