Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
231873006:
Make sure resource client is called asynchronously (Closed)
|
Created:
6 years, 8 months ago by Alpha Left Google Modified:
6 years, 8 months ago CC:
gavinp+loader_chromium.orgm, eseidel, mark a. foltz, Hajime Morrita, adamk Base URL:
svn://svn.chromium.org/blink/trunk Visibility:
Public. |
DescriptionMake sure resource client is called asynchronously
Certain type of resources must have their clients notified
asynchronously.
BUG=352043
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=171402
Patch Set 1 #Patch Set 2 : comments #
Total comments: 4
Patch Set 3 : comments #Patch Set 4 : git cl try #Patch Set 5 : fixed test; it had incorrect expectation #Patch Set 6 : stupid cq won't let me land #Messages
Total messages: 43 (0 generated)
LGTM, so long as the trybots all being red is just a merge conflict :) https://codereview.chromium.org/231873006/diff/20001/Source/core/fetch/RawRes... File Source/core/fetch/RawResourceTest.cpp (right): https://codereview.chromium.org/231873006/diff/20001/Source/core/fetch/RawRes... Source/core/fetch/RawResourceTest.cpp:109: : m_dummyClient(client) Nit: indent intializers. https://codereview.chromium.org/231873006/diff/20001/Source/core/fetch/RawRes... Source/core/fetch/RawResourceTest.cpp:136: ResourcePtr<Resource> rawOwner = raw; Nit: You should be able to merge these two lines and stick the new RawResource straight into the ResourcePtr.
https://codereview.chromium.org/231873006/diff/20001/Source/core/fetch/RawRes... File Source/core/fetch/RawResourceTest.cpp (right): https://codereview.chromium.org/231873006/diff/20001/Source/core/fetch/RawRes... Source/core/fetch/RawResourceTest.cpp:109: : m_dummyClient(client) On 2014/04/10 17:36:10, Nate Chapin wrote: > Nit: indent intializers. Done. https://codereview.chromium.org/231873006/diff/20001/Source/core/fetch/RawRes... Source/core/fetch/RawResourceTest.cpp:136: ResourcePtr<Resource> rawOwner = raw; On 2014/04/10 17:36:10, Nate Chapin wrote: > Nit: You should be able to merge these two lines and stick the new RawResource > straight into the ResourcePtr. Done.
On 2014/04/10 18:26:23, Alpha wrote: > https://codereview.chromium.org/231873006/diff/20001/Source/core/fetch/RawRes... > File Source/core/fetch/RawResourceTest.cpp (right): > > https://codereview.chromium.org/231873006/diff/20001/Source/core/fetch/RawRes... > Source/core/fetch/RawResourceTest.cpp:109: : m_dummyClient(client) > On 2014/04/10 17:36:10, Nate Chapin wrote: > > Nit: indent intializers. > > Done. > > https://codereview.chromium.org/231873006/diff/20001/Source/core/fetch/RawRes... > Source/core/fetch/RawResourceTest.cpp:136: ResourcePtr<Resource> rawOwner = raw; > On 2014/04/10 17:36:10, Nate Chapin wrote: > > Nit: You should be able to merge these two lines and stick the new RawResource > > straight into the ResourcePtr. > > Done. LGTM
The CQ bit was checked by hclam@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/hclam@chromium.org/231873006/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.blink on linux_blink_dbg
The CQ bit was checked by hclam@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/hclam@chromium.org/231873006/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.blink on win_blink_rel
The CQ bit was checked by morrita@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/hclam@chromium.org/231873006/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.blink on linux_blink_dbg
The CQ bit was checked by hclam@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/hclam@chromium.org/231873006/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.blink on linux_blink_dbg
The CQ bit was checked by hclam@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/hclam@chromium.org/231873006/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.blink on linux_blink_dbg
Most bots are completed and green. This is ready for review now.
On 2014/04/11 06:22:58, Alpha wrote: > Most bots are completed and green. This is ready for review now. Still LGTM :)
The CQ bit was checked by hclam@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/hclam@chromium.org/231873006/60002
The CQ bit was unchecked by hclam@chromium.org
The CQ bit was checked by hclam@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/hclam@chromium.org/231873006/60002
The CQ bit was unchecked by hclam@chromium.org
The CQ bit was checked by hclam@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/hclam@chromium.org/231873006/60002
The CQ bit was unchecked by hclam@chromium.org
The CQ bit was checked by hclam@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/hclam@chromium.org/231873006/90001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.blink on mac_blink_rel
The CQ bit was checked by hclam@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/hclam@chromium.org/231873006/90001
Message was sent while issue was closed.
Change committed as 171402
Message was sent while issue was closed.
I wonder how many other places in our codebase we have this problematic pattern. We fought instances like this for mutation events for years. Hopefully this is the last one for a while. |
