Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
158133003:
Move quota module to oilpan (Closed)
|
Created:
6 years, 10 months ago by sof Modified:
6 years, 10 months ago Reviewers:
Vyacheslav Egorov (Chromium), Mads Ager (chromium), tkent, zerny-chromium, kinuko, haraken CC:
blink-reviews, kinuko, oilpan-reviews, nhiroki Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Visibility:
Public. |
DescriptionMove quota module to oilpan
This moves the storage quota interfaces to oilpan, using transition
types.
Mostly regular, apart from one one interface (DeprecatedStorageQuota)
being used from both a main world and worker context, requiring thread
affinity defaults to be overridden for its class.
R=haraken@chromium.org
BUG=340522
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=166819
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=166849
Patch Set 1 #
Total comments: 8
Patch Set 2 : Remove conditional support for GCed-supplements #
Total comments: 24
Patch Set 3 : Revert having empty destructors in header files. #
Total comments: 8
Messages
Total messages: 35 (0 generated)
When you get the chance, please have a look. The #ifdef-ery used on the Supplement classes is really just intended to leave some marker behind so that we don't forget to translate those when the upstream Supplementables are converted over. Is there a standard (&better) way to express that. Please disregard redness of the trybots, unrelated tests currently failing. Tests run ok locally with a enable_oilpan=1 build -- are there any "oilpan-enabled" trybots available for use yet?
Thanks sof for the CL! I'm very excited that we have more contributors to oilpan. I'm happy to reviewing your CLs. https://codereview.chromium.org/158133003/diff/1/Source/modules/quota/DOMWind... File Source/modules/quota/DOMWindowQuota.h (right): https://codereview.chromium.org/158133003/diff/1/Source/modules/quota/DOMWind... Source/modules/quota/DOMWindowQuota.h:46: #define RefPtrWillBePersistentThenMember RefPtrWillBePersistent I understand your intention, but I'm not really happy with introducing more oilpan types and #if macros. Given that it will take a long time to move DOMWindow and Navigator to oilpan, we will end up having the confusing types and macros for a long time in the code base. I'd prefer just using RefPtrWillBePersistent at the moment. Once we move DOMWindow to oilpan, we can just change it to RefPtrWillBeMember. The oilpan system already has an ASSERT to verify that Persistent is not used in on-heap objects. So if we forget to replace RefPtrWillBePersistent with RefPtrMember, we will be asserted. Thus I think it's OK to use RefPtrWillBePersistent at the moment. https://codereview.chromium.org/158133003/diff/1/Source/modules/quota/Depreca... File Source/modules/quota/DeprecatedStorageInfo.h (right): https://codereview.chromium.org/158133003/diff/1/Source/modules/quota/Depreca... Source/modules/quota/DeprecatedStorageInfo.h:48: class DeprecatedStorageInfo : public RefCountedWillBeGarbageCollectedFinalized<DeprecatedStorageInfo>, public ScriptWrappable { Isn't USED_FROM_MULTIPLE_THREADS(DeprecatedStorageInfo) needed? I'm not sure. (This week we'll work on removing USED_FROM_MULTIPLE_THREADS macros completely, since if we forget to add the macro, it will cause a bunch of threading troubles in tests and real world.)
Nit note: Please add "oilpan-reviews@chromium.org" to oilpan CLs.
(+cc nhiroki)
Thanks haraken, I'll try to be useful to the effort. https://codereview.chromium.org/158133003/diff/1/Source/modules/quota/DOMWind... File Source/modules/quota/DOMWindowQuota.h (right): https://codereview.chromium.org/158133003/diff/1/Source/modules/quota/DOMWind... Source/modules/quota/DOMWindowQuota.h:46: #define RefPtrWillBePersistentThenMember RefPtrWillBePersistent On 2014/02/10 01:34:58, haraken wrote: > ... > > The oilpan system already has an ASSERT to verify that Persistent is not used in > on-heap objects. So if we forget to replace RefPtrWillBePersistent with > RefPtrMember, we will be asserted. Thus I think it's OK to use > RefPtrWillBePersistent at the moment. > That works for me; main concern was that we'd accidentally forget about going over the code again once these Supplementables become GCed. I've removed the #ifdefs. https://codereview.chromium.org/158133003/diff/1/Source/modules/quota/Depreca... File Source/modules/quota/DeprecatedStorageInfo.h (right): https://codereview.chromium.org/158133003/diff/1/Source/modules/quota/Depreca... Source/modules/quota/DeprecatedStorageInfo.h:48: class DeprecatedStorageInfo : public RefCountedWillBeGarbageCollectedFinalized<DeprecatedStorageInfo>, public ScriptWrappable { On 2014/02/10 01:34:58, haraken wrote: > > Isn't USED_FROM_MULTIPLE_THREADS(DeprecatedStorageInfo) needed? I'm not sure. > > (This week we'll work on removing USED_FROM_MULTIPLE_THREADS macros completely, > since if we forget to add the macro, it will cause a bunch of threading troubles > in tests and real world.) It shouldn't be needed, as there are no DeprecatedStorageInfo persistents on WorkerNavigatorStorageQuota. Will keep an eye on the changes made in this area; it's quite brittle as it is.
https://codereview.chromium.org/158133003/diff/1/Source/modules/quota/DOMWind... File Source/modules/quota/DOMWindowQuota.h (right): https://codereview.chromium.org/158133003/diff/1/Source/modules/quota/DOMWind... Source/modules/quota/DOMWindowQuota.h:46: #define RefPtrWillBePersistentThenMember RefPtrWillBePersistent On 2014/02/10 07:35:12, sof wrote: > On 2014/02/10 01:34:58, haraken wrote: > > > ... > > > > The oilpan system already has an ASSERT to verify that Persistent is not used > in > > on-heap objects. So if we forget to replace RefPtrWillBePersistent with > > RefPtrMember, we will be asserted. Thus I think it's OK to use > > RefPtrWillBePersistent at the moment. > > > > That works for me; main concern was that we'd accidentally forget about going > over the code again once these Supplementables become GCed. I've removed the > #ifdefs. Yeah, that's a valid concern. Fortunately, if we forget to change Persistent to Member, we will be asserted, so we don't need to worry about it :) https://codereview.chromium.org/158133003/diff/1/Source/modules/quota/Depreca... File Source/modules/quota/DeprecatedStorageInfo.h (right): https://codereview.chromium.org/158133003/diff/1/Source/modules/quota/Depreca... Source/modules/quota/DeprecatedStorageInfo.h:48: class DeprecatedStorageInfo : public RefCountedWillBeGarbageCollectedFinalized<DeprecatedStorageInfo>, public ScriptWrappable { On 2014/02/10 07:35:12, sof wrote: > On 2014/02/10 01:34:58, haraken wrote: > > > > Isn't USED_FROM_MULTIPLE_THREADS(DeprecatedStorageInfo) needed? I'm not sure. > > > > (This week we'll work on removing USED_FROM_MULTIPLE_THREADS macros > completely, > > since if we forget to add the macro, it will cause a bunch of threading > troubles > > in tests and real world.) > > It shouldn't be needed, as there are no DeprecatedStorageInfo persistents on > WorkerNavigatorStorageQuota. > > Will keep an eye on the changes made in this area; it's quite brittle as it is. Thanks, but would you add USED_FROM_MULTIPLE_THREADS if DeprecatedStorageInfo can be potentially used by non-main threads? Otherwise, it will start crashing once we start using Member<DeprecatedStorageInfo> or Persistent<DeprecatedStorageInfo> somewhere. (This rule is fragile and that's why I want to remove USED_FROM_MULTIPLE_THREADS :-)
https://codereview.chromium.org/158133003/diff/1/Source/modules/quota/Depreca... File Source/modules/quota/DeprecatedStorageInfo.h (right): https://codereview.chromium.org/158133003/diff/1/Source/modules/quota/Depreca... Source/modules/quota/DeprecatedStorageInfo.h:48: class DeprecatedStorageInfo : public RefCountedWillBeGarbageCollectedFinalized<DeprecatedStorageInfo>, public ScriptWrappable { On 2014/02/10 07:42:14, haraken wrote: > On 2014/02/10 07:35:12, sof wrote: > > On 2014/02/10 01:34:58, haraken wrote: > > > > > > Isn't USED_FROM_MULTIPLE_THREADS(DeprecatedStorageInfo) needed? I'm not > sure. > > > > > > (This week we'll work on removing USED_FROM_MULTIPLE_THREADS macros > > completely, > > > since if we forget to add the macro, it will cause a bunch of threading > > troubles > > > in tests and real world.) > > > > It shouldn't be needed, as there are no DeprecatedStorageInfo persistents on > > WorkerNavigatorStorageQuota. > > > > Will keep an eye on the changes made in this area; it's quite brittle as it > is. > > Thanks, but would you add USED_FROM_MULTIPLE_THREADS if DeprecatedStorageInfo > can be potentially used by non-main threads? Otherwise, it will start crashing > once we start using Member<DeprecatedStorageInfo> or > Persistent<DeprecatedStorageInfo> somewhere. > > (This rule is fragile and that's why I want to remove USED_FROM_MULTIPLE_THREADS > :-) I would clearly add it if that's a possibility, but I don't see how it can happen overnight here (StorageInfos are only on "window"). Do we want to future-proof many/most objects this way? i.e., I want to understand what the rule is first :)
LGTM kinuko, nhiroki: Would it be OK to land this oilpan change now? It will take a week to completely oilpanize quota/. If you're currently actively developing the code around here, we can wait for landing the CL. https://codereview.chromium.org/158133003/diff/1/Source/modules/quota/Depreca... File Source/modules/quota/DeprecatedStorageInfo.h (right): https://codereview.chromium.org/158133003/diff/1/Source/modules/quota/Depreca... Source/modules/quota/DeprecatedStorageInfo.h:48: class DeprecatedStorageInfo : public RefCountedWillBeGarbageCollectedFinalized<DeprecatedStorageInfo>, public ScriptWrappable { On 2014/02/10 07:57:41, sof wrote: > On 2014/02/10 07:42:14, haraken wrote: > > On 2014/02/10 07:35:12, sof wrote: > > > On 2014/02/10 01:34:58, haraken wrote: > > > > > > > > Isn't USED_FROM_MULTIPLE_THREADS(DeprecatedStorageInfo) needed? I'm not > > sure. > > > > > > > > (This week we'll work on removing USED_FROM_MULTIPLE_THREADS macros > > > completely, > > > > since if we forget to add the macro, it will cause a bunch of threading > > > troubles > > > > in tests and real world.) > > > > > > It shouldn't be needed, as there are no DeprecatedStorageInfo persistents on > > > WorkerNavigatorStorageQuota. > > > > > > Will keep an eye on the changes made in this area; it's quite brittle as it > > is. > > > > Thanks, but would you add USED_FROM_MULTIPLE_THREADS if DeprecatedStorageInfo > > can be potentially used by non-main threads? Otherwise, it will start crashing > > once we start using Member<DeprecatedStorageInfo> or > > Persistent<DeprecatedStorageInfo> somewhere. > > > > (This rule is fragile and that's why I want to remove > USED_FROM_MULTIPLE_THREADS > > :-) > > I would clearly add it if that's a possibility, but I don't see how it can > happen overnight here (StorageInfos are only on "window"). Do we want to > future-proof many/most objects this way? i.e., I want to understand what the > rule is first :) I'm sorry, if it's on window, it shouldn't be touched by workers. Sorry, let's get rid of the macro. In my understanding, a reasonable rule would be "if the object can be touched by workers, we should add the macro".
On 2014/02/10 08:15:22, haraken wrote: > kinuko, nhiroki: Would it be OK to land this oilpan change now? It will take a > week to completely oilpanize quota/. If you're currently actively developing the > code around here, we can wait for landing the CL. No problem for me. Please go ahead.
On 2014/02/10 08:23:46, nhiroki wrote: > On 2014/02/10 08:15:22, haraken wrote: > > kinuko, nhiroki: Would it be OK to land this oilpan change now? It will take a > > week to completely oilpanize quota/. If you're currently actively developing > the > > code around here, we can wait for landing the CL. > > No problem for me. Please go ahead. Thanks; sending it along now.
The CQ bit was checked by sigbjornf@opera.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sigbjornf@opera.com/158133003/220001
https://codereview.chromium.org/158133003/diff/220001/Source/modules/quota/DO... File Source/modules/quota/DOMWindowQuota.cpp (left): https://codereview.chromium.org/158133003/diff/220001/Source/modules/quota/DO... Source/modules/quota/DOMWindowQuota.cpp:56: // static Ditto. https://codereview.chromium.org/158133003/diff/220001/Source/modules/quota/DO... Source/modules/quota/DOMWindowQuota.cpp:67: // static Ditto. https://codereview.chromium.org/158133003/diff/220001/Source/modules/quota/DO... File Source/modules/quota/DOMWindowQuota.h (right): https://codereview.chromium.org/158133003/diff/220001/Source/modules/quota/DO... Source/modules/quota/DOMWindowQuota.h:45: virtual ~DOMWindowQuota() { } Why do you change this? https://codereview.chromium.org/158133003/diff/220001/Source/modules/quota/De... File Source/modules/quota/DeprecatedStorageInfo.h (right): https://codereview.chromium.org/158133003/diff/220001/Source/modules/quota/De... Source/modules/quota/DeprecatedStorageInfo.h:65: ~DeprecatedStorageInfo() { } Ditto. https://codereview.chromium.org/158133003/diff/220001/Source/modules/quota/De... File Source/modules/quota/DeprecatedStorageQuota.h (right): https://codereview.chromium.org/158133003/diff/220001/Source/modules/quota/De... Source/modules/quota/DeprecatedStorageQuota.h:64: ~DeprecatedStorageQuota() { } Ditto. https://codereview.chromium.org/158133003/diff/220001/Source/modules/quota/Na... File Source/modules/quota/NavigatorStorageQuota.h (right): https://codereview.chromium.org/158133003/diff/220001/Source/modules/quota/Na... Source/modules/quota/NavigatorStorageQuota.h:47: virtual ~NavigatorStorageQuota() { } Ditto. https://codereview.chromium.org/158133003/diff/220001/Source/modules/quota/St... File Source/modules/quota/StorageInfo.h (right): https://codereview.chromium.org/158133003/diff/220001/Source/modules/quota/St... Source/modules/quota/StorageInfo.h:53: ~StorageInfo() { } Ditto. https://codereview.chromium.org/158133003/diff/220001/Source/modules/quota/St... File Source/modules/quota/StorageQuota.h (right): https://codereview.chromium.org/158133003/diff/220001/Source/modules/quota/St... Source/modules/quota/StorageQuota.h:58: ~StorageQuota() { } Ditto. https://codereview.chromium.org/158133003/diff/220001/Source/modules/quota/Wo... File Source/modules/quota/WorkerNavigatorStorageQuota.h (right): https://codereview.chromium.org/158133003/diff/220001/Source/modules/quota/Wo... Source/modules/quota/WorkerNavigatorStorageQuota.h:44: virtual ~WorkerNavigatorStorageQuota() { } Ditto. https://codereview.chromium.org/158133003/diff/220001/Source/modules/quota/Wo... Source/modules/quota/WorkerNavigatorStorageQuota.h:53: explicit WorkerNavigatorStorageQuota() { } Ditto.
The CQ bit was unchecked by sigbjornf@opera.com
https://codereview.chromium.org/158133003/diff/220001/Source/modules/quota/DO... File Source/modules/quota/DOMWindowQuota.h (right): https://codereview.chromium.org/158133003/diff/220001/Source/modules/quota/DO... Source/modules/quota/DOMWindowQuota.h:45: virtual ~DOMWindowQuota() { } On 2014/02/10 08:27:40, tkent wrote: > Why do you change this? Why wouldn't you want to see in the header file that the destructor is empty and that (potentially) a *GarbageCollectedFinalized is not needed? i.e., what do you gain from not inlining the empty destructor here (and elsewhere)?
> Source/modules/quota/DOMWindowQuota.h:45: virtual ~DOMWindowQuota() { }
> On 2014/02/10 08:27:40, tkent wrote:
> > Why do you change this?
>
> Why wouldn't you want to see in the header file that the destructor is empty
and
> that (potentially) a *GarbageCollectedFinalized is not needed?
>
> i.e., what do you gain from not inlining the empty destructor here (and
> elsewhere)?
I don't care as long as inlining the empty destructor causes any problem, but it
can sometimes cause a problem in oilpan.
class X {
~X() {} // This requires Y's definition, so you'll need to include Y.h.
Persistent<Y> m_y;
};
This means that you'll sometimes need to explicitly define an out-of-line
destructor when oilpanizing the class (because if you omit an empty destructor,
compilers will generate the inlined destructor in a header file and causes the
above issue).
https://codereview.chromium.org/158133003/diff/220001/Source/modules/quota/DO... File Source/modules/quota/DOMWindowQuota.h (right): https://codereview.chromium.org/158133003/diff/220001/Source/modules/quota/DO... Source/modules/quota/DOMWindowQuota.h:45: virtual ~DOMWindowQuota() { } On 2014/02/10 08:32:13, sof wrote: > On 2014/02/10 08:27:40, tkent wrote: > > Why do you change this? > > Why wouldn't you want to see in the header file that the destructor is empty and > that (potentially) a *GarbageCollectedFinalized is not needed? > > i.e., what do you gain from not inlining the empty destructor here (and > elsewhere)? This CL should focus on Oilapan transition. Changes unnecessary for Oilpan would be noise. Also, virtual destructors can not be inlined. This change has no performance benefit and makes build performance slightly worse.
https://codereview.chromium.org/158133003/diff/220001/Source/modules/quota/DO... File Source/modules/quota/DOMWindowQuota.h (right): https://codereview.chromium.org/158133003/diff/220001/Source/modules/quota/DO... Source/modules/quota/DOMWindowQuota.h:45: virtual ~DOMWindowQuota() { } On 2014/02/10 08:41:57, tkent wrote: > On 2014/02/10 08:32:13, sof wrote: > > On 2014/02/10 08:27:40, tkent wrote: > > > Why do you change this? > > > > Why wouldn't you want to see in the header file that the destructor is empty > and > > that (potentially) a *GarbageCollectedFinalized is not needed? > > > > i.e., what do you gain from not inlining the empty destructor here (and > > elsewhere)? > > This CL should focus on Oilapan transition. Changes unnecessary for Oilpan > would be noise. > Hmm.. ok. > Also, virtual destructors can not be inlined. This change has no performance > benefit and makes build performance slightly worse. > I don't think performance was the argument for it here....
https://codereview.chromium.org/158133003/diff/220001/Source/modules/quota/DO... File Source/modules/quota/DOMWindowQuota.cpp (left): https://codereview.chromium.org/158133003/diff/220001/Source/modules/quota/DO... Source/modules/quota/DOMWindowQuota.cpp:47: DOMWindowQuota::~DOMWindowQuota() Reverted. https://codereview.chromium.org/158133003/diff/220001/Source/modules/quota/DO... Source/modules/quota/DOMWindowQuota.cpp:56: // static On 2014/02/10 08:27:40, tkent wrote: > Ditto. Reverted.. https://codereview.chromium.org/158133003/diff/220001/Source/modules/quota/DO... Source/modules/quota/DOMWindowQuota.cpp:67: // static On 2014/02/10 08:27:40, tkent wrote: > Ditto. Reverted. https://codereview.chromium.org/158133003/diff/220001/Source/modules/quota/DO... File Source/modules/quota/DOMWindowQuota.h (right): https://codereview.chromium.org/158133003/diff/220001/Source/modules/quota/DO... Source/modules/quota/DOMWindowQuota.h:45: virtual ~DOMWindowQuota() { } On 2014/02/10 08:45:09, sof wrote: > On 2014/02/10 08:41:57, tkent wrote: > > On 2014/02/10 08:32:13, sof wrote: > > > On 2014/02/10 08:27:40, tkent wrote: > > > > Why do you change this? > > > > > > Why wouldn't you want to see in the header file that the destructor is empty > > and > > > that (potentially) a *GarbageCollectedFinalized is not needed? > > > > > > i.e., what do you gain from not inlining the empty destructor here (and > > > elsewhere)? > > > > This CL should focus on Oilapan transition. Changes unnecessary for Oilpan > > would be noise. > > > > Hmm.. ok. > > > Also, virtual destructors can not be inlined. This change has no performance > > benefit and makes build performance slightly worse. > > > > I don't think performance was the argument for it here.... > Reverted. https://codereview.chromium.org/158133003/diff/220001/Source/modules/quota/De... File Source/modules/quota/DeprecatedStorageInfo.h (right): https://codereview.chromium.org/158133003/diff/220001/Source/modules/quota/De... Source/modules/quota/DeprecatedStorageInfo.h:65: ~DeprecatedStorageInfo() { } On 2014/02/10 08:27:40, tkent wrote: > Ditto. Reverted. https://codereview.chromium.org/158133003/diff/220001/Source/modules/quota/De... File Source/modules/quota/DeprecatedStorageQuota.h (right): https://codereview.chromium.org/158133003/diff/220001/Source/modules/quota/De... Source/modules/quota/DeprecatedStorageQuota.h:64: ~DeprecatedStorageQuota() { } On 2014/02/10 08:27:40, tkent wrote: > Ditto. Reverted. https://codereview.chromium.org/158133003/diff/220001/Source/modules/quota/Na... File Source/modules/quota/NavigatorStorageQuota.h (right): https://codereview.chromium.org/158133003/diff/220001/Source/modules/quota/Na... Source/modules/quota/NavigatorStorageQuota.h:47: virtual ~NavigatorStorageQuota() { } On 2014/02/10 08:27:40, tkent wrote: > Ditto. Reverted. https://codereview.chromium.org/158133003/diff/220001/Source/modules/quota/St... File Source/modules/quota/StorageInfo.h (right): https://codereview.chromium.org/158133003/diff/220001/Source/modules/quota/St... Source/modules/quota/StorageInfo.h:53: ~StorageInfo() { } On 2014/02/10 08:27:40, tkent wrote: > Ditto. Reverted. https://codereview.chromium.org/158133003/diff/220001/Source/modules/quota/St... File Source/modules/quota/StorageQuota.h (right): https://codereview.chromium.org/158133003/diff/220001/Source/modules/quota/St... Source/modules/quota/StorageQuota.h:58: ~StorageQuota() { } On 2014/02/10 08:27:40, tkent wrote: > Ditto. Reverted. https://codereview.chromium.org/158133003/diff/220001/Source/modules/quota/Wo... File Source/modules/quota/WorkerNavigatorStorageQuota.h (right): https://codereview.chromium.org/158133003/diff/220001/Source/modules/quota/Wo... Source/modules/quota/WorkerNavigatorStorageQuota.h:44: virtual ~WorkerNavigatorStorageQuota() { } On 2014/02/10 08:27:40, tkent wrote: > Ditto. Reverted. https://codereview.chromium.org/158133003/diff/220001/Source/modules/quota/Wo... Source/modules/quota/WorkerNavigatorStorageQuota.h:53: explicit WorkerNavigatorStorageQuota() { } On 2014/02/10 08:27:40, tkent wrote: > Ditto. Reverted.
https://codereview.chromium.org/158133003/diff/430002/Source/modules/quota/De... File Source/modules/quota/DeprecatedStorageInfo.h (right): https://codereview.chromium.org/158133003/diff/430002/Source/modules/quota/De... Source/modules/quota/DeprecatedStorageInfo.h:36: #include "modules/quota/DeprecatedStorageQuota.h" This include looks unnecessary.
https://codereview.chromium.org/158133003/diff/430002/Source/modules/quota/De... File Source/modules/quota/DeprecatedStorageInfo.h (right): https://codereview.chromium.org/158133003/diff/430002/Source/modules/quota/De... Source/modules/quota/DeprecatedStorageInfo.h:36: #include "modules/quota/DeprecatedStorageQuota.h" On 2014/02/10 09:17:17, tkent wrote: > This include looks unnecessary. But it isn't. You need the explicit #include to get the correct ThreadingTrait<> instantiation. Without it, you'll run into redefinitions.
https://codereview.chromium.org/158133003/diff/430002/Source/modules/quota/De... File Source/modules/quota/DeprecatedStorageInfo.h (right): https://codereview.chromium.org/158133003/diff/430002/Source/modules/quota/De... Source/modules/quota/DeprecatedStorageInfo.h:36: #include "modules/quota/DeprecatedStorageQuota.h" On 2014/02/10 09:19:51, sof wrote: > On 2014/02/10 09:17:17, tkent wrote: > > This include looks unnecessary. > > But it isn't. You need the explicit #include to get the correct ThreadingTrait<> > instantiation. Without it, you'll run into redefinitions. Yeah, if we use Member<X>/Persistent<X>, we need to include X.h into the header file... (This has caused #include cycles in our experimental branch. We need to address the cycle case-by-case.)
lgtm. thank you! https://codereview.chromium.org/158133003/diff/430002/Source/modules/quota/De... File Source/modules/quota/DeprecatedStorageInfo.h (right): https://codereview.chromium.org/158133003/diff/430002/Source/modules/quota/De... Source/modules/quota/DeprecatedStorageInfo.h:36: #include "modules/quota/DeprecatedStorageQuota.h" On 2014/02/10 09:19:51, sof wrote: > On 2014/02/10 09:17:17, tkent wrote: > > This include looks unnecessary. > > But it isn't. You need the explicit #include to get the correct ThreadingTrait<> > instantiation. Without it, you'll run into redefinitions. ok, I undetstand.
The CQ bit was checked by tkent@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sigbjornf@opera.com/158133003/430002
Message was sent while issue was closed.
Change committed as 166819
Message was sent while issue was closed.
A revert of this CL has been created in https://codereview.chromium.org/158873002/ by dstockwell@chromium.org. The reason for reverting is: Broke the Oilpan build. See: http://build.chromium.org/p/chromium.webkit/builders/WebKit%20Win%20Oilpan/bu....
With https://codereview.chromium.org/148513008 having landed and the trybots showing green http://build.chromium.org/p/tryserver.chromium/builders/mac_blink_rel/builds/... http://build.chromium.org/p/tryserver.chromium/builders/linux_blink_rel/build... http://build.chromium.org/p/tryserver.chromium/builders/win_blink_rel/builds/... on top of it, I'm trying this one again.
The CQ bit was checked by sigbjornf@opera.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sigbjornf@opera.com/158133003/430002
Message was sent while issue was closed.
Change committed as 166849
Message was sent while issue was closed.
DBC https://codereview.chromium.org/158133003/diff/430002/Source/modules/quota/De... File Source/modules/quota/DeprecatedStorageInfo.h (right): https://codereview.chromium.org/158133003/diff/430002/Source/modules/quota/De... Source/modules/quota/DeprecatedStorageInfo.h:36: #include "modules/quota/DeprecatedStorageQuota.h" Member<X> should not need definition of X as it is just a raw pointer essentially. I am not sure I follow why this was included here. On 2014/02/10 09:28:39, tkent wrote: > On 2014/02/10 09:19:51, sof wrote: > > On 2014/02/10 09:17:17, tkent wrote: > > > This include looks unnecessary. > > > > But it isn't. You need the explicit #include to get the correct > ThreadingTrait<> > > instantiation. Without it, you'll run into redefinitions. > > ok, I undetstand. https://codereview.chromium.org/158133003/diff/430002/Source/modules/quota/De... File Source/modules/quota/DeprecatedStorageQuota.h (right): https://codereview.chromium.org/158133003/diff/430002/Source/modules/quota/De... Source/modules/quota/DeprecatedStorageQuota.h:73: USED_FROM_MULTIPLE_THREADS(DeprecatedStorageQuota); On the branch we were placing this into ThreadState.h to guarantee visibility and avoid including this file into all files that just use Persistent<> of this type.
Message was sent while issue was closed.
https://codereview.chromium.org/158133003/diff/430002/Source/modules/quota/De... File Source/modules/quota/DeprecatedStorageQuota.h (right): https://codereview.chromium.org/158133003/diff/430002/Source/modules/quota/De... Source/modules/quota/DeprecatedStorageQuota.h:73: USED_FROM_MULTIPLE_THREADS(DeprecatedStorageQuota); On 2014/02/10 18:21:05, Vyacheslav Egorov wrote: > On the branch we were placing this into ThreadState.h to guarantee visibility > and avoid including this file into all files that just use Persistent<> of this > type. ah, ok. I notice there's a FIXME just above that list in ThreadState.h ("find a better place for this.") There isn't?
Message was sent while issue was closed.
https://codereview.chromium.org/158133003/diff/430002/Source/modules/quota/De... File Source/modules/quota/DeprecatedStorageQuota.h (right): https://codereview.chromium.org/158133003/diff/430002/Source/modules/quota/De... Source/modules/quota/DeprecatedStorageQuota.h:73: USED_FROM_MULTIPLE_THREADS(DeprecatedStorageQuota); On 2014/02/11 06:54:55, sof wrote: > On 2014/02/10 18:21:05, Vyacheslav Egorov wrote: > > On the branch we were placing this into ThreadState.h to guarantee visibility > > and avoid including this file into all files that just use Persistent<> of > this > > type. > > ah, ok. > > I notice there's a FIXME just above that list in ThreadState.h ("find a better > place for this.") There isn't? Yeah, I think we did not succeed in finding one. We might want to have heap/ThreadAffinityTraits.h and put all of them there and include it into heap/ThreadState.h. That's as good as it gets, I guess.
Message was sent while issue was closed.
On 2014/02/11 11:19:59, Vyacheslav Egorov wrote: > https://codereview.chromium.org/158133003/diff/430002/Source/modules/quota/De... > File Source/modules/quota/DeprecatedStorageQuota.h (right): > > https://codereview.chromium.org/158133003/diff/430002/Source/modules/quota/De... > Source/modules/quota/DeprecatedStorageQuota.h:73: > USED_FROM_MULTIPLE_THREADS(DeprecatedStorageQuota); > On 2014/02/11 06:54:55, sof wrote: > > On 2014/02/10 18:21:05, Vyacheslav Egorov wrote: > > > On the branch we were placing this into ThreadState.h to guarantee > visibility > > > and avoid including this file into all files that just use Persistent<> of > > this > > > type. > > > > ah, ok. > > > > I notice there's a FIXME just above that list in ThreadState.h ("find a better > > place for this.") There isn't? > > Yeah, I think we did not succeed in finding one. > > We might want to have heap/ThreadAffinityTraits.h and put all of them there and > include it into heap/ThreadState.h. That's as good as it gets, I guess. Thanks, yes. Having the do-i-need-custom-thread-affinity question on your checklist when moving code over matters more. |
