Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2394793002:
Resource: helper class to prohibit {add,remove}Client() calls in scope (Closed)
|
Created:
4 years, 2 months ago by Shao-Chuan Lee Modified:
4 years, 2 months ago CC:
chromium-reviews, blink-reviews, loading-reviews+fetch_chromium.org, tyoshino+watch_chromium.org, Nate Chapin, gavinp+loader_chromium.org, Takashi Toyoshima, Kunihiko Sakamoto, kouhei (in TOK) Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionResource: helper class to prohibit {add,remove}Client() calls in scope
Helper class for Resource subclasses to prohibit addClient() and removeClient()
calls e.g. in ResourceClient callbacks.
BUG=570205
Committed: https://crrev.com/d9143c64ebe54aef74585efc460f62441eb96efd
Cr-Commit-Position: refs/heads/master@{#423815}
Patch Set 1 #
Total comments: 14
Patch Set 2 : use MockResourceClient, fixup #Patch Set 3 : rebase #
Messages
Total messages: 30 (19 generated)
shaochuan@chromium.org changed reviewers: + hiroshige@chromium.org, yhirano@chromium.org
Splitting this part off https://crrev.com/2390583002 as suggested by hiroshige@. PTAL
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/v2/patch-status/codereview.chromium.or...
kouhei@chromium.org changed reviewers: + kouhei@chromium.org
Thanks for working on this! https://codereview.chromium.org/2394793002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/fetch/ResourceTest.cpp (right): https://codereview.chromium.org/2394793002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/fetch/ResourceTest.cpp:58: ResourceRequest(), Resource::Raw, ResourceLoaderOptions()); We can just pass these to Resource ctor in line 70. and remove ctor args from line 67. https://codereview.chromium.org/2394793002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/fetch/ResourceTest.cpp:60: void test() { We should have this logic in line 112 func.
https://codereview.chromium.org/2394793002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/fetch/ResourceTest.cpp (right): https://codereview.chromium.org/2394793002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/fetch/ResourceTest.cpp:54: class TestProhibitAddRemoveClientResource : public Resource { optional: I'd like to avoid creating new test-only subclass of Resource, but this is still OK. (Alternatively, we might skip this test and add a new test when we add actual use of ProhibitAddRemoveClientInScope). https://codereview.chromium.org/2394793002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/fetch/ResourceTest.cpp:54: class TestProhibitAddRemoveClientResource : public Resource { Add "final". https://codereview.chromium.org/2394793002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/fetch/ResourceTest.cpp:62: EXPECT_DEATH(addClient(nullptr), "!m_isAddRemoveClientProhibited"); addClient(nullptr) is invalid even outside ProhibitAddRemoveClientInScope, so please use MockResourceClient or something non-null. https://codereview.chromium.org/2394793002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/fetch/ResourceTest.cpp:63: EXPECT_DEATH(removeClient(nullptr), "!m_isAddRemoveClientProhibited"); ditto. https://codereview.chromium.org/2394793002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/fetch/ResourceTest.cpp:67: TestProhibitAddRemoveClientResource(const ResourceRequest& request, We can remove these parameters as they receive only fixed values.
https://codereview.chromium.org/2394793002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/fetch/ResourceTest.cpp (right): https://codereview.chromium.org/2394793002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/fetch/ResourceTest.cpp:62: EXPECT_DEATH(addClient(nullptr), "!m_isAddRemoveClientProhibited"); On 2016/10/05 08:37:25, hiroshige wrote: > addClient(nullptr) is invalid even outside ProhibitAddRemoveClientInScope, so > please use MockResourceClient or something non-null. Since MockResourceClient calls addClient in ctor, I tried EXPECT_DEATH(new MockResourceClient(this), ...) but the problem is EXPECT_DEATH seems to fork another process for the check, then it crashes in Oilpan code (operator new -> enterGCForbiddenScopeIfNeeded() -> checkThread()). To workaround this I'm now checking if stderr matches "!m_isAddRemoveClientProhibited", or we should modify MockResourceClient or have another minimalist ResourceClient subclass.
Adding an empty ctor to MockResourceClient. PTAL https://codereview.chromium.org/2394793002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/fetch/ResourceTest.cpp (right): https://codereview.chromium.org/2394793002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/fetch/ResourceTest.cpp:54: class TestProhibitAddRemoveClientResource : public Resource { On 2016/10/05 08:37:25, hiroshige wrote: > Add "final". Done. https://codereview.chromium.org/2394793002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/fetch/ResourceTest.cpp:58: ResourceRequest(), Resource::Raw, ResourceLoaderOptions()); On 2016/10/05 08:29:42, kouhei wrote: > We can just pass these to Resource ctor in line 70. and remove ctor args from > line 67. Done. https://codereview.chromium.org/2394793002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/fetch/ResourceTest.cpp:60: void test() { On 2016/10/05 08:29:42, kouhei wrote: > We should have this logic in line 112 func. Moving EXPECT_DEATH()s to test function. https://codereview.chromium.org/2394793002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/fetch/ResourceTest.cpp:62: EXPECT_DEATH(addClient(nullptr), "!m_isAddRemoveClientProhibited"); On 2016/10/05 08:51:20, Shao-Chuan Lee wrote: > On 2016/10/05 08:37:25, hiroshige wrote: > > addClient(nullptr) is invalid even outside ProhibitAddRemoveClientInScope, so > > please use MockResourceClient or something non-null. > > Since MockResourceClient calls addClient in ctor, I tried > > EXPECT_DEATH(new MockResourceClient(this), ...) > > but the problem is EXPECT_DEATH seems to fork another process for the check, > then it crashes in Oilpan code (operator new -> enterGCForbiddenScopeIfNeeded() > -> checkThread()). > > To workaround this I'm now checking if stderr matches > "!m_isAddRemoveClientProhibited", or we should modify MockResourceClient or have > another minimalist ResourceClient subclass. Done. https://codereview.chromium.org/2394793002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/fetch/ResourceTest.cpp:63: EXPECT_DEATH(removeClient(nullptr), "!m_isAddRemoveClientProhibited"); On 2016/10/05 08:37:26, hiroshige wrote: > ditto. Done. https://codereview.chromium.org/2394793002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/fetch/ResourceTest.cpp:67: TestProhibitAddRemoveClientResource(const ResourceRequest& request, On 2016/10/05 08:37:25, hiroshige wrote: > We can remove these parameters as they receive only fixed values. Done.
lgtm
lgtm
The CQ bit was checked by shaochuan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by shaochuan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by shaochuan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm!
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 shaochuan@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from yhirano@chromium.org, kouhei@chromium.org Link to the patchset: https://codereview.chromium.org/2394793002/#ps40001 (title: "rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from
==========
Resource: helper class to prohibit {add,remove}Client() calls in scope
Helper class for Resource subclasses to prohibit addClient() and removeClient()
calls e.g. in ResourceClient callbacks.
BUG=570205
==========
to
==========
Resource: helper class to prohibit {add,remove}Client() calls in scope
Helper class for Resource subclasses to prohibit addClient() and removeClient()
calls e.g. in ResourceClient callbacks.
BUG=570205
Committed: https://crrev.com/d9143c64ebe54aef74585efc460f62441eb96efd
Cr-Commit-Position: refs/heads/master@{#423815}
==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/d9143c64ebe54aef74585efc460f62441eb96efd Cr-Commit-Position: refs/heads/master@{#423815} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
