Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
405603002:
DevTools: Merge WorkerInfo into EmbeddedWorkerDevToolsAgentHost (Closed)
|
Created:
6 years, 5 months ago by vkuzkokov Modified:
6 years, 4 months ago CC:
chromium-reviews, vsevik, jam, paulirish+reviews_chromium.org, darin-cc_chromium.org, devtools-reviews_chromium.org, aandrey+blink_chromium.org, pfeldman Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionDevTools: Merge WorkerInfo into EmbeddedWorkerDevToolsAgentHost
Fixes issues with EWDTAH lifetime. Now EWDTAH lives as long as there is a scoped_refptr referencing it or the worker is alive.
Split off https://codereview.chromium.org/349033009/
BUG=389454
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=288362
Patch Set 1 #
Total comments: 2
Patch Set 2 : Mergd WorkerInfo and EmbeddedWorkerDevToolsAgentHost #Patch Set 3 : Renamed MoveToPausedState to WorkerRestarted #
Total comments: 21
Patch Set 4 : Moved WorkerState. Some renames #Patch Set 5 : #
Total comments: 11
Patch Set 6 : Got rid of worker_attached_ #Patch Set 7 : Encapsulated EWDTAH from EWDTM #
Total comments: 2
Patch Set 8 : Fixed nits #Patch Set 9 : Extracted EWDTAH to separate file. Fixed test #Patch Set 10 : Rebased #Patch Set 11 : Style fix #
Total comments: 6
Patch Set 12 : Fixed state transitions during client reattach + test #
Messages
Total messages: 32 (0 generated)
Is there a reason EmbeddedWorkerDevToolsAgentHost doesn't have the exact lifetime as WorkerInfo structure? +horo, who should know the exact reasons. I think we can: - always create EmbeddedWorkerDTAH for WorkerInfo; - make scoped_reftpr from WorkerInfo to DTAH; - remove all the "if agent_host" checks; - make sure it's safe (NoOp) to call DTAH->Attach() when worker is already terminated. https://codereview.chromium.org/405603002/diff/1/content/browser/devtools/emb... File content/browser/devtools/embedded_worker_devtools_manager.cc (right): https://codereview.chromium.org/405603002/diff/1/content/browser/devtools/emb... content/browser/devtools/embedded_worker_devtools_manager.cc:379: agent_host->WorkerCreated(); Why do we call WorkerCreated from within MoveToPausedState?
On 2014/07/21 13:41:16, dgozman wrote: > Is there a reason EmbeddedWorkerDevToolsAgentHost doesn't have the exact > lifetime as WorkerInfo structure? > +horo, who should know the exact reasons. DevToolsAgentHost is only used when the user opens the DevTools. So I thougt that we should create DevToolsAgentHost when the user opens the DevTools not when the worker is created for performance reason. (RenderViewDevToolsAgentHost is created when DevToolsAgentHost::GetOrCreateFor() is called.)
On 2014/07/21 13:41:16, dgozman wrote: > Is there a reason EmbeddedWorkerDevToolsAgentHost doesn't have the exact > lifetime as WorkerInfo structure? > +horo, who should know the exact reasons. > > I think we can: > - always create EmbeddedWorkerDTAH for WorkerInfo; EmbeddedWorkerDTAH is created by lazily, basically the same way as RenderViewDTAH.
Please add an explain in the description which problem this CL fixes. Also it likely needs a test (see embedded_worker_devtools_manager_unittest.cc).
On 2014/07/22 03:37:20, horo wrote: > On 2014/07/21 13:41:16, dgozman wrote: > > Is there a reason EmbeddedWorkerDevToolsAgentHost doesn't have the exact > > lifetime as WorkerInfo structure? > > +horo, who should know the exact reasons. > > DevToolsAgentHost is only used when the user opens the DevTools. > So I thougt that we should create DevToolsAgentHost when the user opens the > DevTools not when the worker is created for performance reason. > (RenderViewDevToolsAgentHost is created when DevToolsAgentHost::GetOrCreateFor() > is called.) But what's the point here? EmbeddedWorkerDTAH is just a couple of bytes in the memory. And it is not destroyed until the worker is terminated. What happens now is: in remote debugging we create EmbeddedWorkerDTAH when listing all the targets, it lives forever, connects to the worker's process and never releases it. We should separate EmbeddedWorkerDTAH lifetime from it's attachment. To simplify things, I propose to always create EmbeddedWorkerDTAH for a worker (treat it as a part of WorkerInfo structure).
On 2014/07/22 15:13:51, dgozman wrote: > On 2014/07/22 03:37:20, horo wrote: > > On 2014/07/21 13:41:16, dgozman wrote: > > > Is there a reason EmbeddedWorkerDevToolsAgentHost doesn't have the exact > > > lifetime as WorkerInfo structure? > > > +horo, who should know the exact reasons. > > > > DevToolsAgentHost is only used when the user opens the DevTools. > > So I thougt that we should create DevToolsAgentHost when the user opens the > > DevTools not when the worker is created for performance reason. > > (RenderViewDevToolsAgentHost is created when > DevToolsAgentHost::GetOrCreateFor() > > is called.) > > But what's the point here? EmbeddedWorkerDTAH is just a couple of bytes in the > memory. And it is not destroyed until the worker is terminated. > What happens now is: in remote debugging we create EmbeddedWorkerDTAH when > listing all the targets, it lives forever, connects to the worker's process and > never releases it. > > We should separate EmbeddedWorkerDTAH lifetime from it's attachment. That should already be the case similar to RenderViewDTAH. > To simplify things, I propose to always create EmbeddedWorkerDTAH for a worker (treat it as > a part of WorkerInfo structure). We used separate WorkerDTAH and WorkerInfo for out-of-process shared workers since it was more convenient to track live worker instances on the IO thread using worker id and created WorkerDevToolsAgentHost on the UI thread when we need one. It may be less useful in case of embedded workers where all worker lifetime notifications are dispatched on the UI thread by embedded shared worker service. Do you propose to do similar change for out-of-process workers as well or should we stick with the current implementation in the latter case?
> > We should separate EmbeddedWorkerDTAH lifetime from it's attachment. > That should already be the case similar to RenderViewDTAH. EmbeddedWorkerDTAH calls AttachToWorker from within constructor, which listens to the process and holds it. > > > To simplify things, I propose to always create EmbeddedWorkerDTAH for a worker > (treat it as > > a part of WorkerInfo structure). > > We used separate WorkerDTAH and WorkerInfo for out-of-process shared workers > since it was more convenient to track live worker instances on the IO thread > using worker id and created WorkerDevToolsAgentHost on the UI thread when we > need one. It may be less useful in case of embedded workers where all worker > lifetime notifications are dispatched on the UI thread by embedded shared worker > service. Do you propose to do similar change for out-of-process workers as well > or should we stick with the current implementation in the latter case? I think out-of-process are fine, since they don't have the same attach problem. Since DTAH is a ref-counted object, we cannot guarantee it's lifetime anyways. So, we can do whatever is easier in each particular case, just ensure that doing stuff with DTAH which has a terminated target is safe.
On 2014/07/22 16:01:30, dgozman wrote: > > > We should separate EmbeddedWorkerDTAH lifetime from it's attachment. > > That should already be the case similar to RenderViewDTAH. > EmbeddedWorkerDTAH calls AttachToWorker from within constructor, which listens > to the process and holds it. > > > > > > To simplify things, I propose to always create EmbeddedWorkerDTAH for a > worker > > (treat it as > > > a part of WorkerInfo structure). > > > > We used separate WorkerDTAH and WorkerInfo for out-of-process shared workers > > since it was more convenient to track live worker instances on the IO thread > > using worker id and created WorkerDevToolsAgentHost on the UI thread when we > > need one. It may be less useful in case of embedded workers where all worker > > lifetime notifications are dispatched on the UI thread by embedded shared > worker > > service. Do you propose to do similar change for out-of-process workers as > well > > or should we stick with the current implementation in the latter case? > I think out-of-process are fine, since they don't have the same attach problem. > Since DTAH is a ref-counted object, we cannot guarantee it's lifetime anyways. > So, we can do whatever is easier in each particular case, just ensure that doing > stuff with DTAH which has a terminated target is safe.
I think it is ok to always create EmbeddedWorkerDTAH since its cost is not too high. I created a patch to remove shared worker process related codes. https://codereview.chromium.org/411283002/ So we don't need to think about the out-of-process shared workers.
On 2014/07/22 16:01:30, dgozman wrote: > > > We should separate EmbeddedWorkerDTAH lifetime from it's attachment. > > That should already be the case similar to RenderViewDTAH. > EmbeddedWorkerDTAH calls AttachToWorker from within constructor, which listens > to the process and holds it. > > > > > > To simplify things, I propose to always create EmbeddedWorkerDTAH for a > worker > > (treat it as > > > a part of WorkerInfo structure). > > > > We used separate WorkerDTAH and WorkerInfo for out-of-process shared workers > > since it was more convenient to track live worker instances on the IO thread > > using worker id and created WorkerDevToolsAgentHost on the UI thread when we > > need one. It may be less useful in case of embedded workers where all worker > > lifetime notifications are dispatched on the UI thread by embedded shared > worker > > service. Do you propose to do similar change for out-of-process workers as > well > > or should we stick with the current implementation in the latter case? > I think out-of-process are fine, since they don't have the same attach problem. > Since DTAH is a ref-counted object, we cannot guarantee it's lifetime anyways. > So, we can do whatever is easier in each particular case, just ensure that doing > stuff with DTAH which has a terminated target is safe. OK, let's merge EmbeddedWorkerDevToolsAgentHost and WorkerInfo then.
WorkerInfo merged into EmbeddedWorkerDevToolsAgentHost. https://codereview.chromium.org/405603002/diff/1/content/browser/devtools/emb... File content/browser/devtools/embedded_worker_devtools_manager.cc (right): https://codereview.chromium.org/405603002/diff/1/content/browser/devtools/emb... content/browser/devtools/embedded_worker_devtools_manager.cc:379: agent_host->WorkerCreated(); On 2014/07/21 13:41:16, dgozman wrote: > Why do we call WorkerCreated from within MoveToPausedState? MoveToPausedState is renamed to WorkerRestarted
https://codereview.chromium.org/405603002/diff/40001/content/browser/devtools... File content/browser/devtools/embedded_worker_devtools_manager.h (right): https://codereview.chromium.org/405603002/diff/40001/content/browser/devtools... content/browser/devtools/embedded_worker_devtools_manager.h:81: enum WorkerState { nit: Move this to .cc. https://codereview.chromium.org/405603002/diff/40001/content/browser/devtools... content/browser/devtools/embedded_worker_devtools_manager.h:89: typedef std::map<WorkerId, EmbeddedWorkerDevToolsAgentHost*> WorkerInfoMap; I think if you use std::map<WorkerId, scoped_refptr<EmbeddedWorkerDevToolsAgentHost>> you don't need to call AddRef() and Release() of EmbeddedWorkerDevToolsAgentHost. https://codereview.chromium.org/405603002/diff/40001/content/browser/devtools... content/browser/devtools/embedded_worker_devtools_manager.h:89: typedef std::map<WorkerId, EmbeddedWorkerDevToolsAgentHost*> WorkerInfoMap; nit: I prefer WorkerAgentHostMap.
This patch needs some comments on the AgentHost lifetime management. Particulary, why there is raw ptr in the map, and why you need AddRef() and Release() in WorkerCreated/Destroyed. https://codereview.chromium.org/405603002/diff/40001/content/browser/devtools... File content/browser/devtools/embedded_worker_devtools_manager.cc (right): https://codereview.chromium.org/405603002/diff/40001/content/browser/devtools... content/browser/devtools/embedded_worker_devtools_manager.cc:188: std::string saved_state_; saved_agent_state_ https://codereview.chromium.org/405603002/diff/40001/content/browser/devtools... content/browser/devtools/embedded_worker_devtools_manager.cc:264: EmbeddedWorkerDevToolsAgentHost* agent = it->second; I'd prefer agent_host. https://codereview.chromium.org/405603002/diff/40001/content/browser/devtools... content/browser/devtools/embedded_worker_devtools_manager.cc:268: agent->set_state(WORKER_TERMINATED); You call set_state(WORKER_TERMINATED) in every case. Move it below the switch? https://codereview.chromium.org/405603002/diff/40001/content/browser/devtools... content/browser/devtools/embedded_worker_devtools_manager.cc:289: workers_.erase(it); This could kill the agent host. https://codereview.chromium.org/405603002/diff/40001/content/browser/devtools... content/browser/devtools/embedded_worker_devtools_manager.cc:319: if (it != workers_.end()) { How is it possible to not find agent host in the map? https://codereview.chromium.org/405603002/diff/40001/content/browser/devtools... content/browser/devtools/embedded_worker_devtools_manager.cc:338: EmbeddedWorkerDevToolsManager::FindExistingSharedWorkerInfo( This is about AgentHost, not WorkerInfo now. https://codereview.chromium.org/405603002/diff/40001/content/browser/devtools... content/browser/devtools/embedded_worker_devtools_manager.cc:349: EmbeddedWorkerDevToolsManager::FindExistingServiceWorkerInfo( This is about AgentHost, not WorkerInfo now.
https://codereview.chromium.org/405603002/diff/40001/content/browser/devtools... File content/browser/devtools/embedded_worker_devtools_manager.cc (right): https://codereview.chromium.org/405603002/diff/40001/content/browser/devtools... content/browser/devtools/embedded_worker_devtools_manager.cc:188: std::string saved_state_; On 2014/08/06 08:13:48, dgozman wrote: > saved_agent_state_ Done. https://codereview.chromium.org/405603002/diff/40001/content/browser/devtools... content/browser/devtools/embedded_worker_devtools_manager.cc:264: EmbeddedWorkerDevToolsAgentHost* agent = it->second; On 2014/08/06 08:13:48, dgozman wrote: > I'd prefer agent_host. Done. https://codereview.chromium.org/405603002/diff/40001/content/browser/devtools... content/browser/devtools/embedded_worker_devtools_manager.cc:268: agent->set_state(WORKER_TERMINATED); On 2014/08/06 08:13:48, dgozman wrote: > You call set_state(WORKER_TERMINATED) in every case. Move it below the switch? Done. https://codereview.chromium.org/405603002/diff/40001/content/browser/devtools... content/browser/devtools/embedded_worker_devtools_manager.cc:289: workers_.erase(it); On 2014/08/06 08:13:48, dgozman wrote: > This could kill the agent host. workers_ stores raw pointers. This shouldn't have any effect on agent hosts. https://codereview.chromium.org/405603002/diff/40001/content/browser/devtools... content/browser/devtools/embedded_worker_devtools_manager.cc:319: if (it != workers_.end()) { On 2014/08/06 08:13:48, dgozman wrote: > How is it possible to not find agent host in the map? It used to be possible (see case WORKER_PAUSED_FOR_REATTACH in WorkerDestroyed). Now it's not the case. https://codereview.chromium.org/405603002/diff/40001/content/browser/devtools... content/browser/devtools/embedded_worker_devtools_manager.cc:338: EmbeddedWorkerDevToolsManager::FindExistingSharedWorkerInfo( On 2014/08/06 08:13:48, dgozman wrote: > This is about AgentHost, not WorkerInfo now. Renamed https://codereview.chromium.org/405603002/diff/40001/content/browser/devtools... content/browser/devtools/embedded_worker_devtools_manager.cc:349: EmbeddedWorkerDevToolsManager::FindExistingServiceWorkerInfo( On 2014/08/06 08:13:48, dgozman wrote: > This is about AgentHost, not WorkerInfo now. Renamed https://codereview.chromium.org/405603002/diff/40001/content/browser/devtools... File content/browser/devtools/embedded_worker_devtools_manager.h (right): https://codereview.chromium.org/405603002/diff/40001/content/browser/devtools... content/browser/devtools/embedded_worker_devtools_manager.h:81: enum WorkerState { On 2014/08/06 07:47:50, horo wrote: > nit: Move this to .cc. Done. https://codereview.chromium.org/405603002/diff/40001/content/browser/devtools... content/browser/devtools/embedded_worker_devtools_manager.h:89: typedef std::map<WorkerId, EmbeddedWorkerDevToolsAgentHost*> WorkerInfoMap; On 2014/08/06 07:47:50, horo wrote: > nit: I prefer WorkerAgentHostMap. AgentHostMap. https://codereview.chromium.org/405603002/diff/40001/content/browser/devtools... content/browser/devtools/embedded_worker_devtools_manager.h:89: typedef std::map<WorkerId, EmbeddedWorkerDevToolsAgentHost*> WorkerInfoMap; On 2014/08/06 07:47:50, horo wrote: > I think if you use > std::map<WorkerId, scoped_refptr<EmbeddedWorkerDevToolsAgentHost>> > you don't need to call AddRef() and Release() of > EmbeddedWorkerDevToolsAgentHost. workers_ keeps live instances of EWDTAH. they are added to workers_ on creation and removed in destructor. workers_ doesn't control lifetime of EWDTAH; it's the opposite. AddRef and Release are unrelated to that. They keep alive agent while its worker is alive.
Please provide meaningful description of the CL.
On 2014/08/06 09:43:09, yurys wrote: > Please provide meaningful description of the CL. Done.
lgtm https://codereview.chromium.org/405603002/diff/40001/content/browser/devtools... File content/browser/devtools/embedded_worker_devtools_manager.h (right): https://codereview.chromium.org/405603002/diff/40001/content/browser/devtools... content/browser/devtools/embedded_worker_devtools_manager.h:89: typedef std::map<WorkerId, EmbeddedWorkerDevToolsAgentHost*> WorkerInfoMap; On 2014/08/06 09:07:39, vkuzkokov wrote: > On 2014/08/06 07:47:50, horo wrote: > > I think if you use > > std::map<WorkerId, scoped_refptr<EmbeddedWorkerDevToolsAgentHost>> > > you don't need to call AddRef() and Release() of > > EmbeddedWorkerDevToolsAgentHost. > > workers_ keeps live instances of EWDTAH. they are added to workers_ on creation > and removed in destructor. workers_ doesn't control lifetime of EWDTAH; it's the > opposite. > > AddRef and Release are unrelated to that. They keep alive agent while its worker > is alive. I see. Sorry I misunderstood
https://codereview.chromium.org/405603002/diff/80001/content/browser/devtools... File content/browser/devtools/embedded_worker_devtools_manager.cc (right): https://codereview.chromium.org/405603002/diff/80001/content/browser/devtools... content/browser/devtools/embedded_worker_devtools_manager.cc:112: virtual void OnClientDetached() OVERRIDE { DetachFromWorker(); } You should check for WORKER_INSPECTED here and move to WORKER_UNINSPECTED. https://codereview.chromium.org/405603002/diff/80001/content/browser/devtools... content/browser/devtools/embedded_worker_devtools_manager.cc:278: agent_host->DetachFromWorker(); Calling DetachFromWorker here is strange. Agent host should not be attached. https://codereview.chromium.org/405603002/diff/80001/content/browser/devtools... content/browser/devtools/embedded_worker_devtools_manager.cc:281: if (agent_host->IsAttached()) { How can agent host not be attached in this case?
https://codereview.chromium.org/405603002/diff/80001/content/browser/devtools... File content/browser/devtools/embedded_worker_devtools_manager.cc (right): https://codereview.chromium.org/405603002/diff/80001/content/browser/devtools... content/browser/devtools/embedded_worker_devtools_manager.cc:239: EmbeddedWorkerDevToolsAgentHost* agent_host = inline agent_host as it is used only once https://codereview.chromium.org/405603002/diff/80001/content/browser/devtools... content/browser/devtools/embedded_worker_devtools_manager.cc:278: agent_host->DetachFromWorker(); On 2014/08/06 10:11:36, dgozman wrote: > Calling DetachFromWorker here is strange. Agent host should not be attached. I agree with Dmitry. Looks like we need a DCHECK here that no one is attached to the EWDTAH https://codereview.chromium.org/405603002/diff/80001/content/browser/devtools... content/browser/devtools/embedded_worker_devtools_manager.cc:298: workers_[agent_host->worker_id()] = agent_host; Isn't it->first == agent_host->worker_id() ?
Got rid of worker_attached_. I use state_ instead. https://codereview.chromium.org/405603002/diff/80001/content/browser/devtools... File content/browser/devtools/embedded_worker_devtools_manager.cc (right): https://codereview.chromium.org/405603002/diff/80001/content/browser/devtools... content/browser/devtools/embedded_worker_devtools_manager.cc:112: virtual void OnClientDetached() OVERRIDE { DetachFromWorker(); } On 2014/08/06 10:11:36, dgozman wrote: > You should check for WORKER_INSPECTED here and move to WORKER_UNINSPECTED. Added to DetachFromWorker https://codereview.chromium.org/405603002/diff/80001/content/browser/devtools... content/browser/devtools/embedded_worker_devtools_manager.cc:239: EmbeddedWorkerDevToolsAgentHost* agent_host = On 2014/08/06 11:26:19, yurys wrote: > inline agent_host as it is used only once Done. https://codereview.chromium.org/405603002/diff/80001/content/browser/devtools... content/browser/devtools/embedded_worker_devtools_manager.cc:278: agent_host->DetachFromWorker(); On 2014/08/06 10:11:36, dgozman wrote: > Calling DetachFromWorker here is strange. Agent host should not be attached. It is strange. Kept as DCHECK. https://codereview.chromium.org/405603002/diff/80001/content/browser/devtools... content/browser/devtools/embedded_worker_devtools_manager.cc:281: if (agent_host->IsAttached()) { On 2014/08/06 10:11:36, dgozman wrote: > How can agent host not be attached in this case? It most likely can't. Fixed. https://codereview.chromium.org/405603002/diff/80001/content/browser/devtools... content/browser/devtools/embedded_worker_devtools_manager.cc:298: workers_[agent_host->worker_id()] = agent_host; On 2014/08/06 11:26:19, yurys wrote: > Isn't it->first == agent_host->worker_id() ? It didn't use to be. Now it is. Fixed.
> https://codereview.chromium.org/405603002/diff/80001/content/browser/devtools... > content/browser/devtools/embedded_worker_devtools_manager.cc:278: > agent_host->DetachFromWorker(); > On 2014/08/06 10:11:36, dgozman wrote: > > Calling DetachFromWorker here is strange. Agent host should not be attached. > > It is strange. Kept as DCHECK. > > https://codereview.chromium.org/405603002/diff/80001/content/browser/devtools... > content/browser/devtools/embedded_worker_devtools_manager.cc:281: if > (agent_host->IsAttached()) { > On 2014/08/06 10:11:36, dgozman wrote: > > How can agent host not be attached in this case? > > It most likely can't. Fixed. Was meant to be: It is strange. Fixed. It most likely can't. Kept as DCHECK.
lgtm https://codereview.chromium.org/405603002/diff/140001/content/browser/devtool... File content/browser/devtools/embedded_worker_devtools_manager.cc (right): https://codereview.chromium.org/405603002/diff/140001/content/browser/devtool... content/browser/devtools/embedded_worker_devtools_manager.cc:153: DCHECK_EQ(state_, WORKER_TERMINATED); DCHECK_EQ (as well as other comparison macros like EXPECT_NE) assume that first parameter is expected to show nice message. https://codereview.chromium.org/405603002/diff/140001/content/browser/devtool... content/browser/devtools/embedded_worker_devtools_manager.cc:186: CHECK_EQ(state_, WORKER_TERMINATED); DCHECK_EQ
The CQ bit was checked by vkuzkokov@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/vkuzkokov@chromium.org/405603002/160001
The CQ bit was unchecked by vkuzkokov@chromium.org
Extracted EWDTAH and fixed test
https://codereview.chromium.org/405603002/diff/220001/content/browser/devtool... File content/browser/devtools/embedded_worker_devtools_agent_host.cc (right): https://codereview.chromium.org/405603002/diff/220001/content/browser/devtool... content/browser/devtools/embedded_worker_devtools_agent_host.cc:64: } else if (state_ == WORKER_PAUSED_FOR_REATTACH) { state_ = WORKER_UNISPECTED; } https://codereview.chromium.org/405603002/diff/220001/content/browser/devtool... content/browser/devtools/embedded_worker_devtools_agent_host.cc:85: } else if (state_ == WORKER_PAUSED_FOR_REATTACH && IsAttached()) { DCHECK(IsAttached()); https://codereview.chromium.org/405603002/diff/220001/content/browser/devtool... content/browser/devtools/embedded_worker_devtools_agent_host.cc:94: state_ = WORKER_PAUSED_FOR_REATTACH; state_ = IsAttached() ? WORKER_PAUSED_FOR_REATTACH : WORKER_UNINSPECTED;
+Test https://codereview.chromium.org/405603002/diff/220001/content/browser/devtool... File content/browser/devtools/embedded_worker_devtools_agent_host.cc (right): https://codereview.chromium.org/405603002/diff/220001/content/browser/devtool... content/browser/devtools/embedded_worker_devtools_agent_host.cc:64: } On 2014/08/08 11:42:12, yurys wrote: > else if (state_ == WORKER_PAUSED_FOR_REATTACH) { > state_ = WORKER_UNISPECTED; > } Done. https://codereview.chromium.org/405603002/diff/220001/content/browser/devtool... content/browser/devtools/embedded_worker_devtools_agent_host.cc:85: } else if (state_ == WORKER_PAUSED_FOR_REATTACH && IsAttached()) { On 2014/08/08 11:42:12, yurys wrote: > DCHECK(IsAttached()); Done. https://codereview.chromium.org/405603002/diff/220001/content/browser/devtool... content/browser/devtools/embedded_worker_devtools_agent_host.cc:94: state_ = WORKER_PAUSED_FOR_REATTACH; On 2014/08/08 11:42:12, yurys wrote: > state_ = IsAttached() ? WORKER_PAUSED_FOR_REATTACH : WORKER_UNINSPECTED; Done.
lgtm
The CQ bit was checked by yurys@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/vkuzkokov@chromium.org/405603002/240001
Message was sent while issue was closed.
Change committed as 288362 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
