Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1807323002:
[WeakMemoryCache 1a] Make Reference from Inspector to Resource weak, remove removedFromMemoryCache() (Closed)
|
Created:
4 years, 9 months ago by hiroshige Modified:
4 years, 7 months ago CC:
chromium-reviews, tyoshino+watch_chromium.org, blink-reviews-style_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, Nate Chapin, kozyatinskiy+blink_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMake Reference from Inspector to Resource weak, remove removedFromMemoryCache()
Previously, Inspector and CSSStyleSheet-related classes depend on whether
Resource is in MemoryCache and track Resource's eviction from MemoryCache by
removedFromMemoryCache().
This CL replaces the condition of "whether Resource is in MemoryCache" with
"whether Resource is not garbage collected":
- Replaces Resource::removedFromMemoryCache() with newly-added Resource's
prefinalizer, Resource::willDestroyResource().
- Make |NetworkResourcesData::ResourceData::m_cachedResource| a WeakMember,
so that Resource's prefinalizer is called even if it is referenced by
|m_cachedResource|.
- Removes some checks of MemoryCache from Inspector, and
replaces StyleSheetContents::isInMemoryCache() with
StyleSheetContents::isReferencedFromResource().
This is Step 1a of Weak MemoryCache:
https://docs.google.com/document/d/1Pj4vzxzCKC_11SugdJ_x7wq7ErE-pHtf4w4FLKdcF7g/edit?usp=sharing
BUG=603462
Committed: https://crrev.com/9b939698486aae30f47b68806ed70d660dea776e
Cr-Commit-Position: refs/heads/master@{#392562}
Patch Set 1 #Patch Set 2 : CSS #Patch Set 3 : Rebase. #Patch Set 4 : Fix. #Patch Set 5 : Fix #Patch Set 6 : #Patch Set 7 : Rebase. #Patch Set 8 : Rebase. #Patch Set 9 : Rebase #Patch Set 10 : Rebase. #Patch Set 11 : Rebase fix. #Patch Set 12 : Rebase. #Patch Set 13 : Rebase #Patch Set 14 : Rebase #Patch Set 15 : Rebase #Patch Set 16 : #
Total comments: 7
Patch Set 17 : Rebase and reflect comments. #Patch Set 18 : auto-Rebase #
Total comments: 4
Patch Set 19 : Remove some comments #Messages
Total messages: 121 (64 generated)
The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1807323002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1807323002/1
The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1807323002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1807323002/20001
Description was changed from ========== Remove removedFromMemoryCache() BUG= ========== to ========== Remove removedFromMemoryCache() BUG= ==========
The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1807323002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1807323002/40001
The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1807323002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1807323002/60001
The CQ bit was unchecked by hiroshige@chromium.org
Patchset #4 (id:60001) has been deleted
Patchset #4 (id:80001) has been deleted
Patchset #4 (id:100001) has been deleted
The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1807323002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1807323002/140001
The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1807323002/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1807323002/160001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
Description was changed from ========== Remove removedFromMemoryCache() BUG= ========== to ========== Make Reference from Inspector to Resource weak, remove removedFromMemoryCache() BUG= ==========
The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1807323002/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1807323002/180001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_blink_oilpan_rel on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_blink_oil...)
The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1807323002/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1807323002/200001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_blink_oilpan_rel on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_blink_oil...)
The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1807323002/260001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1807323002/260001
Description was changed from ========== Make Reference from Inspector to Resource weak, remove removedFromMemoryCache() BUG= ========== to ========== Make Reference from Inspector to Resource weak, remove removedFromMemoryCache() BUG=603462 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1807323002/280001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1807323002/280001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1807323002/300001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1807323002/300001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1807323002/340001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1807323002/340001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run
Patchset #16 (id:360001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1807323002/380001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1807323002/380001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_chromium_gn_compile_dbg on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_chro...)
Description was changed from ========== Make Reference from Inspector to Resource weak, remove removedFromMemoryCache() BUG=603462 ========== to ========== Make Reference from Inspector to Resource weak, remove removedFromMemoryCache() Previously, Inspector and CSSStyleSheet-related classes depend on whether Resource is in MemoryCache and track Resource's eviction from MemoryCache by removedFromMemoryCache(). This CL replaces the condition of "whether Resource is in MemoryCache" with "whether Resource is not garbage collected": - Replaces Resource::removedFromMemoryCache() with newly-added Resource's prefinalizer, Resource::willDestroyResource(). - Make |NetworkResourcesData::ResourceData::m_cachedResource| a WeakMember, so that Resource's prefinalizer is called even if it is referenced by |m_cachedResource|. - Removes some checks of MemoryCache from Inspector, and replaces StyleSheetContents::isInMemoryCache() with StyleSheetContents::isReferencedFromResource(). This is Step 1a of Weak MemoryCache: https://docs.google.com/document/d/1Pj4vzxzCKC_11SugdJ_x7wq7ErE-pHtf4w4FLKdcF... ==========
The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1807323002/380001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1807323002/380001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
hiroshige@chromium.org changed reviewers: + dgozman@chromium.org, japhet@chromium.org, pfeldman@chromium.org, yhirano@chromium.org
PTAL.
lgtm https://codereview.chromium.org/1807323002/diff/380001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/inspector/InspectorResourceAgent.cpp (right): https://codereview.chromium.org/1807323002/diff/380001/third_party/WebKit/Sou... third_party/WebKit/Source/core/inspector/InspectorResourceAgent.cpp:569: // and removed in removedResourceFromMemoryCache(), so if the Resource isn't Looks like this comment needs to be updated or removed.
core/inspector lgtm, but I cannot say anything about all the css changes. I think it makes sense to sync with Oilpan team about WeakMember. https://codereview.chromium.org/1807323002/diff/380001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/fetch/Resource.cpp (right): https://codereview.chromium.org/1807323002/diff/380001/third_party/WebKit/Sou... third_party/WebKit/Source/core/fetch/Resource.cpp:247: InspectorInstrumentation::willDestroyResource(this); Can we have a weak callback in WeakMember instead? Have you checked with Oilpan team, do they plan to support it? It's unfortunate we have to do manual plumbing using global collection in InspectorInstrumentation.cpp.
lgtm https://codereview.chromium.org/1807323002/diff/380001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/css/StyleSheetContents.h (right): https://codereview.chromium.org/1807323002/diff/380001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/StyleSheetContents.h:144: bool isReferencedFromResource() const { return m_isReferencedFromResource; } It may be a good idea to add a comment that a StyleSheetContents must not be shared by multiple resources somewhere.
The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1807323002/400001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1807323002/400001
https://codereview.chromium.org/1807323002/diff/380001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/css/StyleSheetContents.h (right): https://codereview.chromium.org/1807323002/diff/380001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/StyleSheetContents.h:144: bool isReferencedFromResource() const { return m_isReferencedFromResource; } On 2016/04/27 11:00:06, yhirano wrote: > It may be a good idea to add a comment that a StyleSheetContents must not be > shared by multiple resources somewhere. Done. https://codereview.chromium.org/1807323002/diff/380001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/fetch/Resource.cpp (right): https://codereview.chromium.org/1807323002/diff/380001/third_party/WebKit/Sou... third_party/WebKit/Source/core/fetch/Resource.cpp:247: InspectorInstrumentation::willDestroyResource(this); On 2016/04/26 23:58:50, dgozman wrote: > Can we have a weak callback in WeakMember instead? > It's unfortunate we have to do manual plumbing using global collection in InspectorInstrumentation.cpp. Excuse me, could you explain a little more? WeakMember of which class? https://codereview.chromium.org/1807323002/diff/380001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/inspector/InspectorResourceAgent.cpp (right): https://codereview.chromium.org/1807323002/diff/380001/third_party/WebKit/Sou... third_party/WebKit/Source/core/inspector/InspectorResourceAgent.cpp:569: // and removed in removedResourceFromMemoryCache(), so if the Resource isn't On 2016/04/26 17:32:20, Nate Chapin wrote: > Looks like this comment needs to be updated or removed. Done.
hiroshige@chromium.org changed reviewers: + tasak@google.com
+tasak@, could you take another look on core/css?
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/1807323002/diff/380001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/fetch/Resource.cpp (right): https://codereview.chromium.org/1807323002/diff/380001/third_party/WebKit/Sou... third_party/WebKit/Source/core/fetch/Resource.cpp:247: InspectorInstrumentation::willDestroyResource(this); On 2016/04/28 08:06:33, hiroshige wrote: > On 2016/04/26 23:58:50, dgozman wrote: > > Can we have a weak callback in WeakMember instead? > > It's unfortunate we have to do manual plumbing > using global collection in InspectorInstrumentation.cpp. > Excuse me, could you explain a little more? WeakMember of which class? Nevermind. Go ahead with this patch, I will post a queestion to oilpan-dev.
On 2016/04/28 15:17:26, dgozman wrote: > https://codereview.chromium.org/1807323002/diff/380001/third_party/WebKit/Sou... > File third_party/WebKit/Source/core/fetch/Resource.cpp (right): > > https://codereview.chromium.org/1807323002/diff/380001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/fetch/Resource.cpp:247: > InspectorInstrumentation::willDestroyResource(this); > On 2016/04/28 08:06:33, hiroshige wrote: > > On 2016/04/26 23:58:50, dgozman wrote: > > > Can we have a weak callback in WeakMember instead? > > > It's unfortunate we have to do manual plumbing > > using global collection in InspectorInstrumentation.cpp. > > Excuse me, could you explain a little more? WeakMember of which class? > > Nevermind. Go ahead with this patch, I will post a queestion to oilpan-dev. I'm not sure if you can replace the pre-finalizer with a weak callback (i.e., registerWeakMember). Remember that, if you have a WeakMember from X to Y, the weak callback is not called if the X and Y die together. The weak processing runs only when Y dies before X. Given that the inspector wants to do the clean-up unconditionally, I think we need the pre-finalizer (which is unconditionally called when the Resource object gets destructed). LGTM.
The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1807323002/400001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1807323002/400001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1807323002/420001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1807323002/420001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1807323002/420001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1807323002/420001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-gn on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...)
The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1807323002/460001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1807323002/460001
The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1807323002/480001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1807323002/480001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1807323002/480001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1807323002/480001
The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1807323002/500001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1807323002/500001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1807323002/500001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1807323002/500001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1807323002/500001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1807323002/500001
Patchset #18 (id:420001) has been deleted
Patchset #19 (id:460001) has been deleted
Patchset #18 (id:440001) has been deleted
Patchset #18 (id:480001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
hiroshige@chromium.org changed reviewers: + rune@opera.com
rune@, Could you take a look at changes under core/css and core/fetch/CSSStyleSheetResource.h/cpp? I rebased after your https://codereview.chromium.org/1941733002/ and changed |m_isInMemoryCache| (which is cleared when removed from MemoryCache) to |m_isReferencedFromResource| (which is cleared in Resource's prefinalizer). Thanks.
Code lgtm, but you may just remove the comments, I think. https://codereview.chromium.org/1807323002/diff/500001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/css/StyleSheetContents.h (right): https://codereview.chromium.org/1807323002/diff/500001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/StyleSheetContents.h:144: // A StyleSheetContents must not be shared by multiple Resource objects. I don't understand this comment. Isn't this flag about the fact that if StyleSheetContents is cached, we must clone it, not mutate it, from CSSOM since it may be used by another link using the cached resource later on? https://codereview.chromium.org/1807323002/diff/500001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/fetch/CSSStyleSheetResource.h (right): https://codereview.chromium.org/1807323002/diff/500001/third_party/WebKit/Sou... third_party/WebKit/Source/core/fetch/CSSStyleSheetResource.h:84: // Call setParsedStyleSheetCache() to set/clear. I don't think this comment adds much value.
https://codereview.chromium.org/1807323002/diff/500001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/css/StyleSheetContents.h (right): https://codereview.chromium.org/1807323002/diff/500001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/StyleSheetContents.h:144: // A StyleSheetContents must not be shared by multiple Resource objects. On 2016/05/09 07:32:46, rune wrote: > I don't understand this comment. Isn't this flag about the fact that if > StyleSheetContents is cached, we must clone it, not mutate it, from CSSOM since > it may be used by another link using the cached resource later on? I meant that when StyleSheetContents is cached, the StyleSheetContents is referenced by only one Resource, not two or more. I removed the comment because the comment can be confusing, and the assumption is checked by ASSERT() in setReferencedFromResource(). https://codereview.chromium.org/1807323002/diff/500001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/fetch/CSSStyleSheetResource.h (right): https://codereview.chromium.org/1807323002/diff/500001/third_party/WebKit/Sou... third_party/WebKit/Source/core/fetch/CSSStyleSheetResource.h:84: // Call setParsedStyleSheetCache() to set/clear. On 2016/05/09 07:32:47, rune wrote: > I don't think this comment adds much value. Done.
The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1807323002/520001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1807323002/520001
The CQ bit was unchecked by hiroshige@chromium.org
The CQ bit was checked by hiroshige@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from japhet@chromium.org, dgozman@chromium.org, yhirano@chromium.org, haraken@chromium.org, tasak@google.com, rune@opera.com Link to the patchset: https://codereview.chromium.org/1807323002/#ps520001 (title: "Remove some comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1807323002/520001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1807323002/520001
Message was sent while issue was closed.
Description was changed from ========== Make Reference from Inspector to Resource weak, remove removedFromMemoryCache() Previously, Inspector and CSSStyleSheet-related classes depend on whether Resource is in MemoryCache and track Resource's eviction from MemoryCache by removedFromMemoryCache(). This CL replaces the condition of "whether Resource is in MemoryCache" with "whether Resource is not garbage collected": - Replaces Resource::removedFromMemoryCache() with newly-added Resource's prefinalizer, Resource::willDestroyResource(). - Make |NetworkResourcesData::ResourceData::m_cachedResource| a WeakMember, so that Resource's prefinalizer is called even if it is referenced by |m_cachedResource|. - Removes some checks of MemoryCache from Inspector, and replaces StyleSheetContents::isInMemoryCache() with StyleSheetContents::isReferencedFromResource(). This is Step 1a of Weak MemoryCache: https://docs.google.com/document/d/1Pj4vzxzCKC_11SugdJ_x7wq7ErE-pHtf4w4FLKdcF... ========== to ========== Make Reference from Inspector to Resource weak, remove removedFromMemoryCache() Previously, Inspector and CSSStyleSheet-related classes depend on whether Resource is in MemoryCache and track Resource's eviction from MemoryCache by removedFromMemoryCache(). This CL replaces the condition of "whether Resource is in MemoryCache" with "whether Resource is not garbage collected": - Replaces Resource::removedFromMemoryCache() with newly-added Resource's prefinalizer, Resource::willDestroyResource(). - Make |NetworkResourcesData::ResourceData::m_cachedResource| a WeakMember, so that Resource's prefinalizer is called even if it is referenced by |m_cachedResource|. - Removes some checks of MemoryCache from Inspector, and replaces StyleSheetContents::isInMemoryCache() with StyleSheetContents::isReferencedFromResource(). This is Step 1a of Weak MemoryCache: https://docs.google.com/document/d/1Pj4vzxzCKC_11SugdJ_x7wq7ErE-pHtf4w4FLKdcF... ==========
Message was sent while issue was closed.
Committed patchset #19 (id:520001)
Message was sent while issue was closed.
Description was changed from ========== Make Reference from Inspector to Resource weak, remove removedFromMemoryCache() Previously, Inspector and CSSStyleSheet-related classes depend on whether Resource is in MemoryCache and track Resource's eviction from MemoryCache by removedFromMemoryCache(). This CL replaces the condition of "whether Resource is in MemoryCache" with "whether Resource is not garbage collected": - Replaces Resource::removedFromMemoryCache() with newly-added Resource's prefinalizer, Resource::willDestroyResource(). - Make |NetworkResourcesData::ResourceData::m_cachedResource| a WeakMember, so that Resource's prefinalizer is called even if it is referenced by |m_cachedResource|. - Removes some checks of MemoryCache from Inspector, and replaces StyleSheetContents::isInMemoryCache() with StyleSheetContents::isReferencedFromResource(). This is Step 1a of Weak MemoryCache: https://docs.google.com/document/d/1Pj4vzxzCKC_11SugdJ_x7wq7ErE-pHtf4w4FLKdcF... ========== to ========== Make Reference from Inspector to Resource weak, remove removedFromMemoryCache() Previously, Inspector and CSSStyleSheet-related classes depend on whether Resource is in MemoryCache and track Resource's eviction from MemoryCache by removedFromMemoryCache(). This CL replaces the condition of "whether Resource is in MemoryCache" with "whether Resource is not garbage collected": - Replaces Resource::removedFromMemoryCache() with newly-added Resource's prefinalizer, Resource::willDestroyResource(). - Make |NetworkResourcesData::ResourceData::m_cachedResource| a WeakMember, so that Resource's prefinalizer is called even if it is referenced by |m_cachedResource|. - Removes some checks of MemoryCache from Inspector, and replaces StyleSheetContents::isInMemoryCache() with StyleSheetContents::isReferencedFromResource(). This is Step 1a of Weak MemoryCache: https://docs.google.com/document/d/1Pj4vzxzCKC_11SugdJ_x7wq7ErE-pHtf4w4FLKdcF... Committed: https://crrev.com/9b939698486aae30f47b68806ed70d660dea776e Cr-Commit-Position: refs/heads/master@{#392562} ==========
Message was sent while issue was closed.
Patchset 19 (id:??) landed as https://crrev.com/9b939698486aae30f47b68806ed70d660dea776e Cr-Commit-Position: refs/heads/master@{#392562}
Message was sent while issue was closed.
Description was changed from ========== Make Reference from Inspector to Resource weak, remove removedFromMemoryCache() Previously, Inspector and CSSStyleSheet-related classes depend on whether Resource is in MemoryCache and track Resource's eviction from MemoryCache by removedFromMemoryCache(). This CL replaces the condition of "whether Resource is in MemoryCache" with "whether Resource is not garbage collected": - Replaces Resource::removedFromMemoryCache() with newly-added Resource's prefinalizer, Resource::willDestroyResource(). - Make |NetworkResourcesData::ResourceData::m_cachedResource| a WeakMember, so that Resource's prefinalizer is called even if it is referenced by |m_cachedResource|. - Removes some checks of MemoryCache from Inspector, and replaces StyleSheetContents::isInMemoryCache() with StyleSheetContents::isReferencedFromResource(). This is Step 1a of Weak MemoryCache: https://docs.google.com/document/d/1Pj4vzxzCKC_11SugdJ_x7wq7ErE-pHtf4w4FLKdcF... Committed: https://crrev.com/9b939698486aae30f47b68806ed70d660dea776e Cr-Commit-Position: refs/heads/master@{#392562} ========== to ========== Make Reference from Inspector to Resource weak, remove removedFromMemoryCache() Previously, Inspector and CSSStyleSheet-related classes depend on whether Resource is in MemoryCache and track Resource's eviction from MemoryCache by removedFromMemoryCache(). This CL replaces the condition of "whether Resource is in MemoryCache" with "whether Resource is not garbage collected": - Replaces Resource::removedFromMemoryCache() with newly-added Resource's prefinalizer, Resource::willDestroyResource(). - Make |NetworkResourcesData::ResourceData::m_cachedResource| a WeakMember, so that Resource's prefinalizer is called even if it is referenced by |m_cachedResource|. - Removes some checks of MemoryCache from Inspector, and replaces StyleSheetContents::isInMemoryCache() with StyleSheetContents::isReferencedFromResource(). This is Step 1a of Weak MemoryCache: https://docs.google.com/document/d/1Pj4vzxzCKC_11SugdJ_x7wq7ErE-pHtf4w4FLKdcF... BUG=603462 Committed: https://crrev.com/9b939698486aae30f47b68806ed70d660dea776e Cr-Commit-Position: refs/heads/master@{#392562} ========== |
