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

Issues Search
    My Issues | Starred     Open | Closed | All

Issue 1982273002: Merge DocumentThreadableLoader's m_weakPtrFactory into that of RawResourceClient (Closed)

Created:
4 years, 7 months ago by hiroshige
Modified:
4 years, 7 months ago
Reviewers:
Nate Chapin, yhirano
CC:
chromium-reviews, blink-reviews, loading-reviews_chromium.org, tyoshino+watch_chromium.org, Nate Chapin, gavinp+loader_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@Loader_WeakPtrRawResourceClient
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Merge DocumentThreadableLoader's m_weakPtrFactory into that of RawResourceClient BUG=612132

Patch Set 1 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+2 lines, -5 lines) Patch
M third_party/WebKit/Source/core/loader/DocumentThreadableLoader.h View 1 chunk +0 lines, -2 lines 0 comments Download
M third_party/WebKit/Source/core/loader/DocumentThreadableLoader.cpp View 3 chunks +2 lines, -3 lines 0 comments Download

Depends on Patchset:

Issue 1986083002 Patch 1

Messages

Total messages: 9 (4 generated)
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1982273002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1982273002/1
4 years, 7 months ago (2016-05-17 09:03:13 UTC) #2
commit-bot: I haz the power
Dry run: This issue passed the CQ dry run.
4 years, 7 months ago (2016-05-17 11:01:02 UTC) #5
hiroshige
PTAL.
4 years, 7 months ago (2016-05-19 05:18:44 UTC) #7
yhirano
Can you tell me if this change is truly needed or just a cleanup? I ...
4 years, 7 months ago (2016-05-19 06:13:05 UTC) #8
hiroshige
4 years, 7 months ago (2016-05-19 06:18:33 UTC) #9 
On 2016/05/19 06:13:05, yhirano wrote:
> Can you tell me if this change is truly needed or just a cleanup?
Just cleanup, not needed for bug fix.
This CL has some pros/cons:
pros: Remove duplicated WeakPtrFactory in the inheritance chain.
cons: Increases dependency between DocumentThreadableLoader's WeakPtr and
RawResourceClient's WeakPtr, that are added for different reasons and both are
somehow tentative fixes.

> I expect at some point in the future DocumentThreadableLoader will be garbage
> collected while RawResourceClient will NOT be garbage-collected. I'm afraid I
> will have to revert this change at that time.
Then, I think the impact of the cons is larger and we can close this CL (no
commit).

Powered by Google App Engine
This is Rietveld 408576698