Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2565323002:
Stop using callbacks in MemoryCoordinatorProxy (Closed)
DescriptionStop using callbacks in MemoryCoordinatorProxy
Before this CL, we passed callbacks to MemoryCoordinatorProxy to inject logic
from content/ to base/. In this design we have to write a lot of boilerplate
code to expose another logic to base/. Stop using callbacks and use an
interface class to avoid such boilerplate code. We are planning to add
another method to MemoryCoordinatorProxy so that we can show the state of each
process in the task manager.
Also, this CL changes GetCurrentMemoryState() to GetLocalMemoryState() as
haraken@ suggested[1].
[1] https://codereview.chromium.org/2564623004/diff/1/content/browser/memory/memory_coordinator.cc#newcode91
BUG=673216
Patch Set 1 #
Total comments: 4
Patch Set 2 : fix #
Total comments: 11
Messages
Total messages: 34 (11 generated)
The CQ bit was checked by bashi@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...
Description was changed from ========== Stop using callbacks in MemoryCoordinatorProxy Before this CL, we passed callbacks to MemoryCoordinatorProxy to inject logic from content/ to base/. In this design we have to write a lot of boilerplate code to expose another logic to base/. Stop using callbacks and use an interface class to avoid such boilerplate code. We are planning to add another method to MemoryCoordinatorProxy so that we can show the state of each process in the task manager. Also, this CL changes GetCurrentMemoryState() to GetLocalMemoryState() as haraken@ suggested[1]. [1] https://codereview.chromium.org/2564623004/diff/1/content/browser/memory/memo... BUG= ========== to ========== Stop using callbacks in MemoryCoordinatorProxy Before this CL, we passed callbacks to MemoryCoordinatorProxy to inject logic from content/ to base/. In this design we have to write a lot of boilerplate code to expose another logic to base/. Stop using callbacks and use an interface class to avoid such boilerplate code. We are planning to add another method to MemoryCoordinatorProxy so that we can show the state of each process in the task manager. Also, this CL changes GetCurrentMemoryState() to GetLocalMemoryState() as haraken@ suggested[1]. [1] https://codereview.chromium.org/2564623004/diff/1/content/browser/memory/memo... BUG=673216 ==========
bashi@chromium.org changed reviewers: + chrisha@chromium.org, haraken@chromium.org
What do you think about this change? I'm trying to expose a method which returns the memory state of a given ProcessHandle (or ProcessId) to MemoryCoordinatorProxy. Adding another callback wouldn't make sense.
LGTM https://codereview.chromium.org/2565323002/diff/1/base/memory/memory_coordina... File base/memory/memory_coordinator_proxy.h (right): https://codereview.chromium.org/2565323002/diff/1/base/memory/memory_coordina... base/memory/memory_coordinator_proxy.h:40: void SetMemoryStateForTesting(MemoryState memory_state); This sets the global memory state, not the memory state of this process, right? SetGlobalMemoryStateForTesting ? https://codereview.chromium.org/2565323002/diff/1/content/browser/memory/memo... File content/browser/memory/memory_coordinator.cc (right): https://codereview.chromium.org/2565323002/diff/1/content/browser/memory/memo... content/browser/memory/memory_coordinator.cc:142: return nullptr; NOT_REACHED() ?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by bashi@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...
https://codereview.chromium.org/2565323002/diff/1/base/memory/memory_coordina... File base/memory/memory_coordinator_proxy.h (right): https://codereview.chromium.org/2565323002/diff/1/base/memory/memory_coordina... base/memory/memory_coordinator_proxy.h:40: void SetMemoryStateForTesting(MemoryState memory_state); On 2016/12/12 06:51:57, haraken wrote: > > This sets the global memory state, not the memory state of this process, right? > > SetGlobalMemoryStateForTesting ? This is a tricky part in some reasons. First, from caller's perspective, this may look a bit strange: SetGlobalMemoryState(NORMAL) ... EXPECT_EQ(NORMAL, GetLocalMemoryState()) Second, thinking about renderer side, there wouldn't be a way to set the global state, so above code doesn't look correct (note that proxy exists to provide common interface to both browser/renderers). FWIW I was thinking about moving GetGlobalMemoryState() into this class, but I didn't do that because of the same reason. That said, there are the same problem for a method which I'm going to add in follow-up CLs. In follow-up CLs, I'll add something like below: MemoryState GetStateForProcess(ProcessHandle); This wouldn't work on renderer side. I was thinking better ways to avoid it, but I couldn't come up with better ideas (Moving MemoryCoordinator to content/public is another idea, but it adds another abstraction layer). https://codereview.chromium.org/2565323002/diff/1/content/browser/memory/memo... File content/browser/memory/memory_coordinator.cc (right): https://codereview.chromium.org/2565323002/diff/1/content/browser/memory/memo... content/browser/memory/memory_coordinator.cc:142: return nullptr; On 2016/12/12 06:51:57, haraken wrote: > > NOT_REACHED() ? Done.
bashi@chromium.org changed reviewers: + dcheng@chromium.org, jam@chromium.org, sky@chromium.org
+sky@ chrome/browser/sessions +jam@ for content/ +dcheng@ for base/
On 2016/12/12 07:17:55, bashi1 wrote: > https://codereview.chromium.org/2565323002/diff/1/base/memory/memory_coordina... > File base/memory/memory_coordinator_proxy.h (right): > > https://codereview.chromium.org/2565323002/diff/1/base/memory/memory_coordina... > base/memory/memory_coordinator_proxy.h:40: void > SetMemoryStateForTesting(MemoryState memory_state); > On 2016/12/12 06:51:57, haraken wrote: > > > > This sets the global memory state, not the memory state of this process, > right? > > > > SetGlobalMemoryStateForTesting ? > > This is a tricky part in some reasons. First, from caller's perspective, this > may look a bit strange: > > SetGlobalMemoryState(NORMAL) > ... > EXPECT_EQ(NORMAL, GetLocalMemoryState()) > > Second, thinking about renderer side, there wouldn't be a way to set the global > state, so above code doesn't look correct (note that proxy exists to provide > common interface to both browser/renderers). > > FWIW I was thinking about moving GetGlobalMemoryState() into this class, but I > didn't do that because of the same reason. > > That said, there are the same problem for a method which I'm going to add in > follow-up CLs. In follow-up CLs, I'll add something like below: > > MemoryState GetStateForProcess(ProcessHandle); > > This wouldn't work on renderer side. I was thinking better ways to avoid it, but > I couldn't come up with better ideas (Moving MemoryCoordinator to content/public > is another idea, but it adds another abstraction layer). So SetMemoryStateForTesting sets a global state when called on a browser but sets a local state when called on a renderer?
https://codereview.chromium.org/2565323002/diff/20001/base/memory/memory_coor... File base/memory/memory_coordinator_proxy.h (right): https://codereview.chromium.org/2565323002/diff/20001/base/memory/memory_coor... base/memory/memory_coordinator_proxy.h:21: virtual MemoryState GetLocalMemoryState() = 0; Two nits: 1) I think it's more common to document the interface and not document the implementation. Should we do that here? 2) It's not very common to have "Interface" in the names of interfaces. I think it's more common to just have the interface be called "MemoryCoordinator" and have the implementation suffixed with Impl so it's called MemoryCoordinatorImpl. https://codereview.chromium.org/2565323002/diff/20001/base/memory/memory_coor... base/memory/memory_coordinator_proxy.h:32: void Set(WeakPtr<MemoryCoordinatorInterface> instance); Does this need to be a weak pointer? Maybe it should just unregister itself on destruction by calling this with nullptr?
On 2016/12/12 07:26:19, haraken wrote: > On 2016/12/12 07:17:55, bashi1 wrote: > > > https://codereview.chromium.org/2565323002/diff/1/base/memory/memory_coordina... > > File base/memory/memory_coordinator_proxy.h (right): > > > > > https://codereview.chromium.org/2565323002/diff/1/base/memory/memory_coordina... > > base/memory/memory_coordinator_proxy.h:40: void > > SetMemoryStateForTesting(MemoryState memory_state); > > On 2016/12/12 06:51:57, haraken wrote: > > > > > > This sets the global memory state, not the memory state of this process, > > right? > > > > > > SetGlobalMemoryStateForTesting ? > > > > This is a tricky part in some reasons. First, from caller's perspective, this > > may look a bit strange: > > > > SetGlobalMemoryState(NORMAL) > > ... > > EXPECT_EQ(NORMAL, GetLocalMemoryState()) > > > > Second, thinking about renderer side, there wouldn't be a way to set the > global > > state, so above code doesn't look correct (note that proxy exists to provide > > common interface to both browser/renderers). > > > > FWIW I was thinking about moving GetGlobalMemoryState() into this class, but I > > didn't do that because of the same reason. > > > > That said, there are the same problem for a method which I'm going to add in > > follow-up CLs. In follow-up CLs, I'll add something like below: > > > > MemoryState GetStateForProcess(ProcessHandle); > > > > This wouldn't work on renderer side. I was thinking better ways to avoid it, > but > > I couldn't come up with better ideas (Moving MemoryCoordinator to > content/public > > is another idea, but it adds another abstraction layer). > > So SetMemoryStateForTesting sets a global state when called on a browser but > sets a local state when called on a renderer? - On browser: sets the global state - On renderer: we don't set callbacks Above is the current implementation but the behavior depends on what we want to do (as long as the naming isn't confusing for both browser/renderers). Setting a local state on renderers sounds reasonable of course.
https://codereview.chromium.org/2565323002/diff/20001/base/memory/memory_coor... File base/memory/memory_coordinator_proxy.h (right): https://codereview.chromium.org/2565323002/diff/20001/base/memory/memory_coor... base/memory/memory_coordinator_proxy.h:21: virtual MemoryState GetLocalMemoryState() = 0; On 2016/12/12 07:27:07, dcheng wrote: > Two nits: > 1) I think it's more common to document the interface and not document the > implementation. Should we do that here? Let me clarify your comments. Do you mean moving comments from Proxy to here? > > 2) It's not very common to have "Interface" in the names of interfaces. I think > it's more common to just have the interface be called "MemoryCoordinator" and > have the implementation suffixed with Impl so it's called MemoryCoordinatorImpl. Yeah, but I can find such naming[1]. I totally agree with your comments but we already have MemoryCoordinator and MemoryCoordinatorImpl in content/. Do you want us to rename them before this CL? What kind of names we should in that case? [1] https://cs.chromium.org/chromium/src/chrome/browser/task_manager/task_manager... https://codereview.chromium.org/2565323002/diff/20001/base/memory/memory_coor... base/memory/memory_coordinator_proxy.h:32: void Set(WeakPtr<MemoryCoordinatorInterface> instance); On 2016/12/12 07:27:07, dcheng wrote: > Does this need to be a weak pointer? Maybe it should just unregister itself on > destruction by calling this with nullptr? Sure. But I think it would be better to have explicit logic to avoid use-after-free rather than putting comments like "must be removed on destruction". WDYT?
On 2016/12/12 07:31:36, bashi1 wrote: > On 2016/12/12 07:26:19, haraken wrote: > > On 2016/12/12 07:17:55, bashi1 wrote: > > > > > > https://codereview.chromium.org/2565323002/diff/1/base/memory/memory_coordina... > > > File base/memory/memory_coordinator_proxy.h (right): > > > > > > > > > https://codereview.chromium.org/2565323002/diff/1/base/memory/memory_coordina... > > > base/memory/memory_coordinator_proxy.h:40: void > > > SetMemoryStateForTesting(MemoryState memory_state); > > > On 2016/12/12 06:51:57, haraken wrote: > > > > > > > > This sets the global memory state, not the memory state of this process, > > > right? > > > > > > > > SetGlobalMemoryStateForTesting ? > > > > > > This is a tricky part in some reasons. First, from caller's perspective, > this > > > may look a bit strange: > > > > > > SetGlobalMemoryState(NORMAL) > > > ... > > > EXPECT_EQ(NORMAL, GetLocalMemoryState()) > > > > > > Second, thinking about renderer side, there wouldn't be a way to set the > > global > > > state, so above code doesn't look correct (note that proxy exists to provide > > > common interface to both browser/renderers). > > > > > > FWIW I was thinking about moving GetGlobalMemoryState() into this class, but > I > > > didn't do that because of the same reason. > > > > > > That said, there are the same problem for a method which I'm going to add in > > > follow-up CLs. In follow-up CLs, I'll add something like below: > > > > > > MemoryState GetStateForProcess(ProcessHandle); > > > > > > This wouldn't work on renderer side. I was thinking better ways to avoid it, > > but > > > I couldn't come up with better ideas (Moving MemoryCoordinator to > > content/public > > > is another idea, but it adds another abstraction layer). > > > > So SetMemoryStateForTesting sets a global state when called on a browser but > > sets a local state when called on a renderer? > > - On browser: sets the global state > - On renderer: we don't set callbacks > > Above is the current implementation but the behavior depends on what we want to > do (as long as the naming isn't confusing for both browser/renderers). Setting a > local state on renderers sounds reasonable of course. Can we add SetGlobalMemoryStateForTesting and SetLocalMemoryStateForTesting? We can assert that SetGlobalMemoryStateForTesting is not called on a renderer or don't set the callback in the first place.
On 2016/12/12 07:39:11, haraken wrote: > On 2016/12/12 07:31:36, bashi1 wrote: > > On 2016/12/12 07:26:19, haraken wrote: > > > On 2016/12/12 07:17:55, bashi1 wrote: > > > > > > > > > > https://codereview.chromium.org/2565323002/diff/1/base/memory/memory_coordina... > > > > File base/memory/memory_coordinator_proxy.h (right): > > > > > > > > > > > > > > https://codereview.chromium.org/2565323002/diff/1/base/memory/memory_coordina... > > > > base/memory/memory_coordinator_proxy.h:40: void > > > > SetMemoryStateForTesting(MemoryState memory_state); > > > > On 2016/12/12 06:51:57, haraken wrote: > > > > > > > > > > This sets the global memory state, not the memory state of this process, > > > > right? > > > > > > > > > > SetGlobalMemoryStateForTesting ? > > > > > > > > This is a tricky part in some reasons. First, from caller's perspective, > > this > > > > may look a bit strange: > > > > > > > > SetGlobalMemoryState(NORMAL) > > > > ... > > > > EXPECT_EQ(NORMAL, GetLocalMemoryState()) > > > > > > > > Second, thinking about renderer side, there wouldn't be a way to set the > > > global > > > > state, so above code doesn't look correct (note that proxy exists to > provide > > > > common interface to both browser/renderers). > > > > > > > > FWIW I was thinking about moving GetGlobalMemoryState() into this class, > but > > I > > > > didn't do that because of the same reason. > > > > > > > > That said, there are the same problem for a method which I'm going to add > in > > > > follow-up CLs. In follow-up CLs, I'll add something like below: > > > > > > > > MemoryState GetStateForProcess(ProcessHandle); > > > > > > > > This wouldn't work on renderer side. I was thinking better ways to avoid > it, > > > but > > > > I couldn't come up with better ideas (Moving MemoryCoordinator to > > > content/public > > > > is another idea, but it adds another abstraction layer). > > > > > > So SetMemoryStateForTesting sets a global state when called on a browser but > > > sets a local state when called on a renderer? > > > > - On browser: sets the global state > > - On renderer: we don't set callbacks > > > > Above is the current implementation but the behavior depends on what we want > to > > do (as long as the naming isn't confusing for both browser/renderers). Setting > a > > local state on renderers sounds reasonable of course. > > Can we add SetGlobalMemoryStateForTesting and SetLocalMemoryStateForTesting? We > can assert that SetGlobalMemoryStateForTesting is not called on a renderer or > don't set the callback in the first place. Could you elaborate why we want to have both?
On 2016/12/12 07:40:43, bashi1 wrote: > On 2016/12/12 07:39:11, haraken wrote: > > On 2016/12/12 07:31:36, bashi1 wrote: > > > On 2016/12/12 07:26:19, haraken wrote: > > > > On 2016/12/12 07:17:55, bashi1 wrote: > > > > > > > > > > > > > > > https://codereview.chromium.org/2565323002/diff/1/base/memory/memory_coordina... > > > > > File base/memory/memory_coordinator_proxy.h (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2565323002/diff/1/base/memory/memory_coordina... > > > > > base/memory/memory_coordinator_proxy.h:40: void > > > > > SetMemoryStateForTesting(MemoryState memory_state); > > > > > On 2016/12/12 06:51:57, haraken wrote: > > > > > > > > > > > > This sets the global memory state, not the memory state of this > process, > > > > > right? > > > > > > > > > > > > SetGlobalMemoryStateForTesting ? > > > > > > > > > > This is a tricky part in some reasons. First, from caller's perspective, > > > this > > > > > may look a bit strange: > > > > > > > > > > SetGlobalMemoryState(NORMAL) > > > > > ... > > > > > EXPECT_EQ(NORMAL, GetLocalMemoryState()) > > > > > > > > > > Second, thinking about renderer side, there wouldn't be a way to set the > > > > global > > > > > state, so above code doesn't look correct (note that proxy exists to > > provide > > > > > common interface to both browser/renderers). > > > > > > > > > > FWIW I was thinking about moving GetGlobalMemoryState() into this class, > > but > > > I > > > > > didn't do that because of the same reason. > > > > > > > > > > That said, there are the same problem for a method which I'm going to > add > > in > > > > > follow-up CLs. In follow-up CLs, I'll add something like below: > > > > > > > > > > MemoryState GetStateForProcess(ProcessHandle); > > > > > > > > > > This wouldn't work on renderer side. I was thinking better ways to avoid > > it, > > > > but > > > > > I couldn't come up with better ideas (Moving MemoryCoordinator to > > > > content/public > > > > > is another idea, but it adds another abstraction layer). > > > > > > > > So SetMemoryStateForTesting sets a global state when called on a browser > but > > > > sets a local state when called on a renderer? > > > > > > - On browser: sets the global state > > > - On renderer: we don't set callbacks > > > > > > Above is the current implementation but the behavior depends on what we want > > to > > > do (as long as the naming isn't confusing for both browser/renderers). > Setting > > a > > > local state on renderers sounds reasonable of course. > > > > Can we add SetGlobalMemoryStateForTesting and SetLocalMemoryStateForTesting? > We > > can assert that SetGlobalMemoryStateForTesting is not called on a renderer or > > don't set the callback in the first place. > > Could you elaborate why we want to have both? (Maybe renaming to LocalState itself isn't a good idea)
On 2016/12/12 07:40:43, bashi1 wrote: > On 2016/12/12 07:39:11, haraken wrote: > > On 2016/12/12 07:31:36, bashi1 wrote: > > > On 2016/12/12 07:26:19, haraken wrote: > > > > On 2016/12/12 07:17:55, bashi1 wrote: > > > > > > > > > > > > > > > https://codereview.chromium.org/2565323002/diff/1/base/memory/memory_coordina... > > > > > File base/memory/memory_coordinator_proxy.h (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2565323002/diff/1/base/memory/memory_coordina... > > > > > base/memory/memory_coordinator_proxy.h:40: void > > > > > SetMemoryStateForTesting(MemoryState memory_state); > > > > > On 2016/12/12 06:51:57, haraken wrote: > > > > > > > > > > > > This sets the global memory state, not the memory state of this > process, > > > > > right? > > > > > > > > > > > > SetGlobalMemoryStateForTesting ? > > > > > > > > > > This is a tricky part in some reasons. First, from caller's perspective, > > > this > > > > > may look a bit strange: > > > > > > > > > > SetGlobalMemoryState(NORMAL) > > > > > ... > > > > > EXPECT_EQ(NORMAL, GetLocalMemoryState()) > > > > > > > > > > Second, thinking about renderer side, there wouldn't be a way to set the > > > > global > > > > > state, so above code doesn't look correct (note that proxy exists to > > provide > > > > > common interface to both browser/renderers). > > > > > > > > > > FWIW I was thinking about moving GetGlobalMemoryState() into this class, > > but > > > I > > > > > didn't do that because of the same reason. > > > > > > > > > > That said, there are the same problem for a method which I'm going to > add > > in > > > > > follow-up CLs. In follow-up CLs, I'll add something like below: > > > > > > > > > > MemoryState GetStateForProcess(ProcessHandle); > > > > > > > > > > This wouldn't work on renderer side. I was thinking better ways to avoid > > it, > > > > but > > > > > I couldn't come up with better ideas (Moving MemoryCoordinator to > > > > content/public > > > > > is another idea, but it adds another abstraction layer). > > > > > > > > So SetMemoryStateForTesting sets a global state when called on a browser > but > > > > sets a local state when called on a renderer? > > > > > > - On browser: sets the global state > > > - On renderer: we don't set callbacks > > > > > > Above is the current implementation but the behavior depends on what we want > > to > > > do (as long as the naming isn't confusing for both browser/renderers). > Setting > > a > > > local state on renderers sounds reasonable of course. > > > > Can we add SetGlobalMemoryStateForTesting and SetLocalMemoryStateForTesting? > We > > can assert that SetGlobalMemoryStateForTesting is not called on a renderer or > > don't set the callback in the first place. > > Could you elaborate why we want to have both? From your message, I thought you want to add SetLocalMemoryStateForTesting in the future. If we only need SetGlobalMemoryStateForTesting, what's an issue of just renaming SetMemoryStateForTesting to SetGlobalMemoryStateForTesting?
On 2016/12/12 07:42:26, haraken wrote: > On 2016/12/12 07:40:43, bashi1 wrote: > > On 2016/12/12 07:39:11, haraken wrote: > > > On 2016/12/12 07:31:36, bashi1 wrote: > > > > On 2016/12/12 07:26:19, haraken wrote: > > > > > On 2016/12/12 07:17:55, bashi1 wrote: > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2565323002/diff/1/base/memory/memory_coordina... > > > > > > File base/memory/memory_coordinator_proxy.h (right): > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2565323002/diff/1/base/memory/memory_coordina... > > > > > > base/memory/memory_coordinator_proxy.h:40: void > > > > > > SetMemoryStateForTesting(MemoryState memory_state); > > > > > > On 2016/12/12 06:51:57, haraken wrote: > > > > > > > > > > > > > > This sets the global memory state, not the memory state of this > > process, > > > > > > right? > > > > > > > > > > > > > > SetGlobalMemoryStateForTesting ? > > > > > > > > > > > > This is a tricky part in some reasons. First, from caller's > perspective, > > > > this > > > > > > may look a bit strange: > > > > > > > > > > > > SetGlobalMemoryState(NORMAL) > > > > > > ... > > > > > > EXPECT_EQ(NORMAL, GetLocalMemoryState()) > > > > > > > > > > > > Second, thinking about renderer side, there wouldn't be a way to set > the > > > > > global > > > > > > state, so above code doesn't look correct (note that proxy exists to > > > provide > > > > > > common interface to both browser/renderers). > > > > > > > > > > > > FWIW I was thinking about moving GetGlobalMemoryState() into this > class, > > > but > > > > I > > > > > > didn't do that because of the same reason. > > > > > > > > > > > > That said, there are the same problem for a method which I'm going to > > add > > > in > > > > > > follow-up CLs. In follow-up CLs, I'll add something like below: > > > > > > > > > > > > MemoryState GetStateForProcess(ProcessHandle); > > > > > > > > > > > > This wouldn't work on renderer side. I was thinking better ways to > avoid > > > it, > > > > > but > > > > > > I couldn't come up with better ideas (Moving MemoryCoordinator to > > > > > content/public > > > > > > is another idea, but it adds another abstraction layer). > > > > > > > > > > So SetMemoryStateForTesting sets a global state when called on a browser > > but > > > > > sets a local state when called on a renderer? > > > > > > > > - On browser: sets the global state > > > > - On renderer: we don't set callbacks > > > > > > > > Above is the current implementation but the behavior depends on what we > want > > > to > > > > do (as long as the naming isn't confusing for both browser/renderers). > > Setting > > > a > > > > local state on renderers sounds reasonable of course. > > > > > > Can we add SetGlobalMemoryStateForTesting and SetLocalMemoryStateForTesting? > > We > > > can assert that SetGlobalMemoryStateForTesting is not called on a renderer > or > > > don't set the callback in the first place. > > > > Could you elaborate why we want to have both? > > From your message, I thought you want to add SetLocalMemoryStateForTesting in > the future. > > If we only need SetGlobalMemoryStateForTesting, what's an issue of just renaming > SetMemoryStateForTesting to SetGlobalMemoryStateForTesting? Having meaningless methods in base/ sounds a bad idea.
On 2016/12/12 07:44:28, bashi1 wrote: > On 2016/12/12 07:42:26, haraken wrote: > > On 2016/12/12 07:40:43, bashi1 wrote: > > > On 2016/12/12 07:39:11, haraken wrote: > > > > On 2016/12/12 07:31:36, bashi1 wrote: > > > > > On 2016/12/12 07:26:19, haraken wrote: > > > > > > On 2016/12/12 07:17:55, bashi1 wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2565323002/diff/1/base/memory/memory_coordina... > > > > > > > File base/memory/memory_coordinator_proxy.h (right): > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2565323002/diff/1/base/memory/memory_coordina... > > > > > > > base/memory/memory_coordinator_proxy.h:40: void > > > > > > > SetMemoryStateForTesting(MemoryState memory_state); > > > > > > > On 2016/12/12 06:51:57, haraken wrote: > > > > > > > > > > > > > > > > This sets the global memory state, not the memory state of this > > > process, > > > > > > > right? > > > > > > > > > > > > > > > > SetGlobalMemoryStateForTesting ? > > > > > > > > > > > > > > This is a tricky part in some reasons. First, from caller's > > perspective, > > > > > this > > > > > > > may look a bit strange: > > > > > > > > > > > > > > SetGlobalMemoryState(NORMAL) > > > > > > > ... > > > > > > > EXPECT_EQ(NORMAL, GetLocalMemoryState()) > > > > > > > > > > > > > > Second, thinking about renderer side, there wouldn't be a way to set > > the > > > > > > global > > > > > > > state, so above code doesn't look correct (note that proxy exists to > > > > provide > > > > > > > common interface to both browser/renderers). > > > > > > > > > > > > > > FWIW I was thinking about moving GetGlobalMemoryState() into this > > class, > > > > but > > > > > I > > > > > > > didn't do that because of the same reason. > > > > > > > > > > > > > > That said, there are the same problem for a method which I'm going > to > > > add > > > > in > > > > > > > follow-up CLs. In follow-up CLs, I'll add something like below: > > > > > > > > > > > > > > MemoryState GetStateForProcess(ProcessHandle); > > > > > > > > > > > > > > This wouldn't work on renderer side. I was thinking better ways to > > avoid > > > > it, > > > > > > but > > > > > > > I couldn't come up with better ideas (Moving MemoryCoordinator to > > > > > > content/public > > > > > > > is another idea, but it adds another abstraction layer). > > > > > > > > > > > > So SetMemoryStateForTesting sets a global state when called on a > browser > > > but > > > > > > sets a local state when called on a renderer? > > > > > > > > > > - On browser: sets the global state > > > > > - On renderer: we don't set callbacks > > > > > > > > > > Above is the current implementation but the behavior depends on what we > > want > > > > to > > > > > do (as long as the naming isn't confusing for both browser/renderers). > > > Setting > > > > a > > > > > local state on renderers sounds reasonable of course. > > > > > > > > Can we add SetGlobalMemoryStateForTesting and > SetLocalMemoryStateForTesting? > > > We > > > > can assert that SetGlobalMemoryStateForTesting is not called on a renderer > > or > > > > don't set the callback in the first place. > > > > > > Could you elaborate why we want to have both? > > > > From your message, I thought you want to add SetLocalMemoryStateForTesting in > > the future. > > > > If we only need SetGlobalMemoryStateForTesting, what's an issue of just > renaming > > SetMemoryStateForTesting to SetGlobalMemoryStateForTesting? > > Having meaningless methods in base/ sounds a bad idea. I'm confused... If you don't need SetLocalMemoryStateForTesting, what's an issue of just renaming SetMemoryStateForTesting to SetGlobalMemoryStateForTesting?
On 2016/12/12 07:45:47, haraken wrote: > On 2016/12/12 07:44:28, bashi1 wrote: > > On 2016/12/12 07:42:26, haraken wrote: > > > On 2016/12/12 07:40:43, bashi1 wrote: > > > > On 2016/12/12 07:39:11, haraken wrote: > > > > > On 2016/12/12 07:31:36, bashi1 wrote: > > > > > > On 2016/12/12 07:26:19, haraken wrote: > > > > > > > On 2016/12/12 07:17:55, bashi1 wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2565323002/diff/1/base/memory/memory_coordina... > > > > > > > > File base/memory/memory_coordinator_proxy.h (right): > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2565323002/diff/1/base/memory/memory_coordina... > > > > > > > > base/memory/memory_coordinator_proxy.h:40: void > > > > > > > > SetMemoryStateForTesting(MemoryState memory_state); > > > > > > > > On 2016/12/12 06:51:57, haraken wrote: > > > > > > > > > > > > > > > > > > This sets the global memory state, not the memory state of this > > > > process, > > > > > > > > right? > > > > > > > > > > > > > > > > > > SetGlobalMemoryStateForTesting ? > > > > > > > > > > > > > > > > This is a tricky part in some reasons. First, from caller's > > > perspective, > > > > > > this > > > > > > > > may look a bit strange: > > > > > > > > > > > > > > > > SetGlobalMemoryState(NORMAL) > > > > > > > > ... > > > > > > > > EXPECT_EQ(NORMAL, GetLocalMemoryState()) > > > > > > > > > > > > > > > > Second, thinking about renderer side, there wouldn't be a way to > set > > > the > > > > > > > global > > > > > > > > state, so above code doesn't look correct (note that proxy exists > to > > > > > provide > > > > > > > > common interface to both browser/renderers). > > > > > > > > > > > > > > > > FWIW I was thinking about moving GetGlobalMemoryState() into this > > > class, > > > > > but > > > > > > I > > > > > > > > didn't do that because of the same reason. > > > > > > > > > > > > > > > > That said, there are the same problem for a method which I'm going > > to > > > > add > > > > > in > > > > > > > > follow-up CLs. In follow-up CLs, I'll add something like below: > > > > > > > > > > > > > > > > MemoryState GetStateForProcess(ProcessHandle); > > > > > > > > > > > > > > > > This wouldn't work on renderer side. I was thinking better ways to > > > avoid > > > > > it, > > > > > > > but > > > > > > > > I couldn't come up with better ideas (Moving MemoryCoordinator to > > > > > > > content/public > > > > > > > > is another idea, but it adds another abstraction layer). > > > > > > > > > > > > > > So SetMemoryStateForTesting sets a global state when called on a > > browser > > > > but > > > > > > > sets a local state when called on a renderer? > > > > > > > > > > > > - On browser: sets the global state > > > > > > - On renderer: we don't set callbacks > > > > > > > > > > > > Above is the current implementation but the behavior depends on what > we > > > want > > > > > to > > > > > > do (as long as the naming isn't confusing for both browser/renderers). > > > > Setting > > > > > a > > > > > > local state on renderers sounds reasonable of course. > > > > > > > > > > Can we add SetGlobalMemoryStateForTesting and > > SetLocalMemoryStateForTesting? > > > > We > > > > > can assert that SetGlobalMemoryStateForTesting is not called on a > renderer > > > or > > > > > don't set the callback in the first place. > > > > > > > > Could you elaborate why we want to have both? > > > > > > From your message, I thought you want to add SetLocalMemoryStateForTesting > in > > > the future. > > > > > > If we only need SetGlobalMemoryStateForTesting, what's an issue of just > > renaming > > > SetMemoryStateForTesting to SetGlobalMemoryStateForTesting? > > > > Having meaningless methods in base/ sounds a bad idea. > > I'm confused... If you don't need SetLocalMemoryStateForTesting, what's an issue > of just renaming SetMemoryStateForTesting to SetGlobalMemoryStateForTesting? Let me clarify my point: - MemoryCoordinatorProxy is a common interface between browser/renderer - SetGlobalMemoryState doesn't make sense on renderer - If we want to have browser specific logic, it should be in content/browser - Why we have SetMemoryState -> We want to set state outside content/ for testing - Why base/ then? -> some reviewers didn't want to put interface code in components/ - (kind a off topic: As I wrote in the previous message, the same problem can be said for GetMemoryStateForProcess(), but I couldn't come up with better idea to avoid it)
On 2016/12/12 07:51:45, bashi1 wrote: > On 2016/12/12 07:45:47, haraken wrote: > > On 2016/12/12 07:44:28, bashi1 wrote: > > > On 2016/12/12 07:42:26, haraken wrote: > > > > On 2016/12/12 07:40:43, bashi1 wrote: > > > > > On 2016/12/12 07:39:11, haraken wrote: > > > > > > On 2016/12/12 07:31:36, bashi1 wrote: > > > > > > > On 2016/12/12 07:26:19, haraken wrote: > > > > > > > > On 2016/12/12 07:17:55, bashi1 wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2565323002/diff/1/base/memory/memory_coordina... > > > > > > > > > File base/memory/memory_coordinator_proxy.h (right): > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2565323002/diff/1/base/memory/memory_coordina... > > > > > > > > > base/memory/memory_coordinator_proxy.h:40: void > > > > > > > > > SetMemoryStateForTesting(MemoryState memory_state); > > > > > > > > > On 2016/12/12 06:51:57, haraken wrote: > > > > > > > > > > > > > > > > > > > > This sets the global memory state, not the memory state of > this > > > > > process, > > > > > > > > > right? > > > > > > > > > > > > > > > > > > > > SetGlobalMemoryStateForTesting ? > > > > > > > > > > > > > > > > > > This is a tricky part in some reasons. First, from caller's > > > > perspective, > > > > > > > this > > > > > > > > > may look a bit strange: > > > > > > > > > > > > > > > > > > SetGlobalMemoryState(NORMAL) > > > > > > > > > ... > > > > > > > > > EXPECT_EQ(NORMAL, GetLocalMemoryState()) > > > > > > > > > > > > > > > > > > Second, thinking about renderer side, there wouldn't be a way to > > set > > > > the > > > > > > > > global > > > > > > > > > state, so above code doesn't look correct (note that proxy > exists > > to > > > > > > provide > > > > > > > > > common interface to both browser/renderers). > > > > > > > > > > > > > > > > > > FWIW I was thinking about moving GetGlobalMemoryState() into > this > > > > class, > > > > > > but > > > > > > > I > > > > > > > > > didn't do that because of the same reason. > > > > > > > > > > > > > > > > > > That said, there are the same problem for a method which I'm > going > > > to > > > > > add > > > > > > in > > > > > > > > > follow-up CLs. In follow-up CLs, I'll add something like below: > > > > > > > > > > > > > > > > > > MemoryState GetStateForProcess(ProcessHandle); > > > > > > > > > > > > > > > > > > This wouldn't work on renderer side. I was thinking better ways > to > > > > avoid > > > > > > it, > > > > > > > > but > > > > > > > > > I couldn't come up with better ideas (Moving MemoryCoordinator > to > > > > > > > > content/public > > > > > > > > > is another idea, but it adds another abstraction layer). > > > > > > > > > > > > > > > > So SetMemoryStateForTesting sets a global state when called on a > > > browser > > > > > but > > > > > > > > sets a local state when called on a renderer? > > > > > > > > > > > > > > - On browser: sets the global state > > > > > > > - On renderer: we don't set callbacks > > > > > > > > > > > > > > Above is the current implementation but the behavior depends on what > > we > > > > want > > > > > > to > > > > > > > do (as long as the naming isn't confusing for both > browser/renderers). > > > > > Setting > > > > > > a > > > > > > > local state on renderers sounds reasonable of course. > > > > > > > > > > > > Can we add SetGlobalMemoryStateForTesting and > > > SetLocalMemoryStateForTesting? > > > > > We > > > > > > can assert that SetGlobalMemoryStateForTesting is not called on a > > renderer > > > > or > > > > > > don't set the callback in the first place. > > > > > > > > > > Could you elaborate why we want to have both? > > > > > > > > From your message, I thought you want to add SetLocalMemoryStateForTesting > > in > > > > the future. > > > > > > > > If we only need SetGlobalMemoryStateForTesting, what's an issue of just > > > renaming > > > > SetMemoryStateForTesting to SetGlobalMemoryStateForTesting? > > > > > > Having meaningless methods in base/ sounds a bad idea. > > > > I'm confused... If you don't need SetLocalMemoryStateForTesting, what's an > issue > > of just renaming SetMemoryStateForTesting to SetGlobalMemoryStateForTesting? > > Let me clarify my point: > - MemoryCoordinatorProxy is a common interface between browser/renderer > - SetGlobalMemoryState doesn't make sense on renderer > - If we want to have browser specific logic, it should be in content/browser > - Why we have SetMemoryState -> We want to set state outside content/ for > testing > - Why base/ then? -> some reviewers didn't want to put interface code in > components/ > - (kind a off topic: As I wrote in the previous message, the same problem can be > said for GetMemoryStateForProcess(), but I couldn't come up with better idea to > avoid it) Thanks for clarifying. Then SetGlobalMemoryStateOnBrowser and GetGlobalMemoryStateOnBrowser? I feel it a bit confusing to put a method like SetMemoryState on MemoryCoordinatorProxy even though it doesn't work in renderer.
On 2016/12/12 08:03:50, haraken wrote: > On 2016/12/12 07:51:45, bashi1 wrote: > > On 2016/12/12 07:45:47, haraken wrote: > > > On 2016/12/12 07:44:28, bashi1 wrote: > > > > On 2016/12/12 07:42:26, haraken wrote: > > > > > On 2016/12/12 07:40:43, bashi1 wrote: > > > > > > On 2016/12/12 07:39:11, haraken wrote: > > > > > > > On 2016/12/12 07:31:36, bashi1 wrote: > > > > > > > > On 2016/12/12 07:26:19, haraken wrote: > > > > > > > > > On 2016/12/12 07:17:55, bashi1 wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2565323002/diff/1/base/memory/memory_coordina... > > > > > > > > > > File base/memory/memory_coordinator_proxy.h (right): > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2565323002/diff/1/base/memory/memory_coordina... > > > > > > > > > > base/memory/memory_coordinator_proxy.h:40: void > > > > > > > > > > SetMemoryStateForTesting(MemoryState memory_state); > > > > > > > > > > On 2016/12/12 06:51:57, haraken wrote: > > > > > > > > > > > > > > > > > > > > > > This sets the global memory state, not the memory state of > > this > > > > > > process, > > > > > > > > > > right? > > > > > > > > > > > > > > > > > > > > > > SetGlobalMemoryStateForTesting ? > > > > > > > > > > > > > > > > > > > > This is a tricky part in some reasons. First, from caller's > > > > > perspective, > > > > > > > > this > > > > > > > > > > may look a bit strange: > > > > > > > > > > > > > > > > > > > > SetGlobalMemoryState(NORMAL) > > > > > > > > > > ... > > > > > > > > > > EXPECT_EQ(NORMAL, GetLocalMemoryState()) > > > > > > > > > > > > > > > > > > > > Second, thinking about renderer side, there wouldn't be a way > to > > > set > > > > > the > > > > > > > > > global > > > > > > > > > > state, so above code doesn't look correct (note that proxy > > exists > > > to > > > > > > > provide > > > > > > > > > > common interface to both browser/renderers). > > > > > > > > > > > > > > > > > > > > FWIW I was thinking about moving GetGlobalMemoryState() into > > this > > > > > class, > > > > > > > but > > > > > > > > I > > > > > > > > > > didn't do that because of the same reason. > > > > > > > > > > > > > > > > > > > > That said, there are the same problem for a method which I'm > > going > > > > to > > > > > > add > > > > > > > in > > > > > > > > > > follow-up CLs. In follow-up CLs, I'll add something like > below: > > > > > > > > > > > > > > > > > > > > MemoryState GetStateForProcess(ProcessHandle); > > > > > > > > > > > > > > > > > > > > This wouldn't work on renderer side. I was thinking better > ways > > to > > > > > avoid > > > > > > > it, > > > > > > > > > but > > > > > > > > > > I couldn't come up with better ideas (Moving MemoryCoordinator > > to > > > > > > > > > content/public > > > > > > > > > > is another idea, but it adds another abstraction layer). > > > > > > > > > > > > > > > > > > So SetMemoryStateForTesting sets a global state when called on a > > > > browser > > > > > > but > > > > > > > > > sets a local state when called on a renderer? > > > > > > > > > > > > > > > > - On browser: sets the global state > > > > > > > > - On renderer: we don't set callbacks > > > > > > > > > > > > > > > > Above is the current implementation but the behavior depends on > what > > > we > > > > > want > > > > > > > to > > > > > > > > do (as long as the naming isn't confusing for both > > browser/renderers). > > > > > > Setting > > > > > > > a > > > > > > > > local state on renderers sounds reasonable of course. > > > > > > > > > > > > > > Can we add SetGlobalMemoryStateForTesting and > > > > SetLocalMemoryStateForTesting? > > > > > > We > > > > > > > can assert that SetGlobalMemoryStateForTesting is not called on a > > > renderer > > > > > or > > > > > > > don't set the callback in the first place. > > > > > > > > > > > > Could you elaborate why we want to have both? > > > > > > > > > > From your message, I thought you want to add > SetLocalMemoryStateForTesting > > > in > > > > > the future. > > > > > > > > > > If we only need SetGlobalMemoryStateForTesting, what's an issue of just > > > > renaming > > > > > SetMemoryStateForTesting to SetGlobalMemoryStateForTesting? > > > > > > > > Having meaningless methods in base/ sounds a bad idea. > > > > > > I'm confused... If you don't need SetLocalMemoryStateForTesting, what's an > > issue > > > of just renaming SetMemoryStateForTesting to SetGlobalMemoryStateForTesting? > > > > Let me clarify my point: > > - MemoryCoordinatorProxy is a common interface between browser/renderer > > - SetGlobalMemoryState doesn't make sense on renderer > > - If we want to have browser specific logic, it should be in content/browser > > - Why we have SetMemoryState -> We want to set state outside content/ for > > testing > > - Why base/ then? -> some reviewers didn't want to put interface code in > > components/ > > - (kind a off topic: As I wrote in the previous message, the same problem can > be > > said for GetMemoryStateForProcess(), but I couldn't come up with better idea > to > > avoid it) > > Thanks for clarifying. Then SetGlobalMemoryStateOnBrowser and > GetGlobalMemoryStateOnBrowser? Hmm, I doubt we want to add browser-specific naming/logic in base/. > > I feel it a bit confusing to put a method like SetMemoryState on > MemoryCoordinatorProxy even though it doesn't work in renderer. I think the problem is to use local/global naming.
On 2016/12/12 08:10:53, bashi1 wrote: > On 2016/12/12 08:03:50, haraken wrote: > > On 2016/12/12 07:51:45, bashi1 wrote: > > > On 2016/12/12 07:45:47, haraken wrote: > > > > On 2016/12/12 07:44:28, bashi1 wrote: > > > > > On 2016/12/12 07:42:26, haraken wrote: > > > > > > On 2016/12/12 07:40:43, bashi1 wrote: > > > > > > > On 2016/12/12 07:39:11, haraken wrote: > > > > > > > > On 2016/12/12 07:31:36, bashi1 wrote: > > > > > > > > > On 2016/12/12 07:26:19, haraken wrote: > > > > > > > > > > On 2016/12/12 07:17:55, bashi1 wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2565323002/diff/1/base/memory/memory_coordina... > > > > > > > > > > > File base/memory/memory_coordinator_proxy.h (right): > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2565323002/diff/1/base/memory/memory_coordina... > > > > > > > > > > > base/memory/memory_coordinator_proxy.h:40: void > > > > > > > > > > > SetMemoryStateForTesting(MemoryState memory_state); > > > > > > > > > > > On 2016/12/12 06:51:57, haraken wrote: > > > > > > > > > > > > > > > > > > > > > > > > This sets the global memory state, not the memory state of > > > this > > > > > > > process, > > > > > > > > > > > right? > > > > > > > > > > > > > > > > > > > > > > > > SetGlobalMemoryStateForTesting ? > > > > > > > > > > > > > > > > > > > > > > This is a tricky part in some reasons. First, from caller's > > > > > > perspective, > > > > > > > > > this > > > > > > > > > > > may look a bit strange: > > > > > > > > > > > > > > > > > > > > > > SetGlobalMemoryState(NORMAL) > > > > > > > > > > > ... > > > > > > > > > > > EXPECT_EQ(NORMAL, GetLocalMemoryState()) > > > > > > > > > > > > > > > > > > > > > > Second, thinking about renderer side, there wouldn't be a > way > > to > > > > set > > > > > > the > > > > > > > > > > global > > > > > > > > > > > state, so above code doesn't look correct (note that proxy > > > exists > > > > to > > > > > > > > provide > > > > > > > > > > > common interface to both browser/renderers). > > > > > > > > > > > > > > > > > > > > > > FWIW I was thinking about moving GetGlobalMemoryState() into > > > this > > > > > > class, > > > > > > > > but > > > > > > > > > I > > > > > > > > > > > didn't do that because of the same reason. > > > > > > > > > > > > > > > > > > > > > > That said, there are the same problem for a method which I'm > > > going > > > > > to > > > > > > > add > > > > > > > > in > > > > > > > > > > > follow-up CLs. In follow-up CLs, I'll add something like > > below: > > > > > > > > > > > > > > > > > > > > > > MemoryState GetStateForProcess(ProcessHandle); > > > > > > > > > > > > > > > > > > > > > > This wouldn't work on renderer side. I was thinking better > > ways > > > to > > > > > > avoid > > > > > > > > it, > > > > > > > > > > but > > > > > > > > > > > I couldn't come up with better ideas (Moving > MemoryCoordinator > > > to > > > > > > > > > > content/public > > > > > > > > > > > is another idea, but it adds another abstraction layer). > > > > > > > > > > > > > > > > > > > > So SetMemoryStateForTesting sets a global state when called on > a > > > > > browser > > > > > > > but > > > > > > > > > > sets a local state when called on a renderer? > > > > > > > > > > > > > > > > > > - On browser: sets the global state > > > > > > > > > - On renderer: we don't set callbacks > > > > > > > > > > > > > > > > > > Above is the current implementation but the behavior depends on > > what > > > > we > > > > > > want > > > > > > > > to > > > > > > > > > do (as long as the naming isn't confusing for both > > > browser/renderers). > > > > > > > Setting > > > > > > > > a > > > > > > > > > local state on renderers sounds reasonable of course. > > > > > > > > > > > > > > > > Can we add SetGlobalMemoryStateForTesting and > > > > > SetLocalMemoryStateForTesting? > > > > > > > We > > > > > > > > can assert that SetGlobalMemoryStateForTesting is not called on a > > > > renderer > > > > > > or > > > > > > > > don't set the callback in the first place. > > > > > > > > > > > > > > Could you elaborate why we want to have both? > > > > > > > > > > > > From your message, I thought you want to add > > SetLocalMemoryStateForTesting > > > > in > > > > > > the future. > > > > > > > > > > > > If we only need SetGlobalMemoryStateForTesting, what's an issue of > just > > > > > renaming > > > > > > SetMemoryStateForTesting to SetGlobalMemoryStateForTesting? > > > > > > > > > > Having meaningless methods in base/ sounds a bad idea. > > > > > > > > I'm confused... If you don't need SetLocalMemoryStateForTesting, what's an > > > issue > > > > of just renaming SetMemoryStateForTesting to > SetGlobalMemoryStateForTesting? > > > > > > Let me clarify my point: > > > - MemoryCoordinatorProxy is a common interface between browser/renderer > > > - SetGlobalMemoryState doesn't make sense on renderer > > > - If we want to have browser specific logic, it should be in content/browser > > > - Why we have SetMemoryState -> We want to set state outside content/ for > > > testing > > > - Why base/ then? -> some reviewers didn't want to put interface code in > > > components/ > > > - (kind a off topic: As I wrote in the previous message, the same problem > can > > be > > > said for GetMemoryStateForProcess(), but I couldn't come up with better idea > > to > > > avoid it) > > > > Thanks for clarifying. Then SetGlobalMemoryStateOnBrowser and > > GetGlobalMemoryStateOnBrowser? > Hmm, I doubt we want to add browser-specific naming/logic in base/. > > > > I feel it a bit confusing to put a method like SetMemoryState on > > MemoryCoordinatorProxy even though it doesn't work in renderer. > I think the problem is to use local/global naming. ok, go ahead
https://codereview.chromium.org/2565323002/diff/20001/base/memory/memory_coor... File base/memory/memory_coordinator_proxy.h (right): https://codereview.chromium.org/2565323002/diff/20001/base/memory/memory_coor... base/memory/memory_coordinator_proxy.h:21: virtual MemoryState GetLocalMemoryState() = 0; On 2016/12/12 07:38:39, bashi1 wrote: > On 2016/12/12 07:27:07, dcheng wrote: > > Two nits: > > 1) I think it's more common to document the interface and not document the > > implementation. Should we do that here? > Let me clarify your comments. Do you mean moving comments from Proxy to here? Yes. > > > > 2) It's not very common to have "Interface" in the names of interfaces. I > think > > it's more common to just have the interface be called "MemoryCoordinator" and > > have the implementation suffixed with Impl so it's called > MemoryCoordinatorImpl. > > Yeah, but I can find such naming[1]. I totally agree with your comments but we > already have MemoryCoordinator and MemoryCoordinatorImpl in content/. Do you > want us to rename them before this CL? What kind of names we should in that > case? > > [1] > https://cs.chromium.org/chromium/src/chrome/browser/task_manager/task_manager... If it would be hard for this CL, it's OK to defer. But I would like to think about improving the names in a followup patch: generally speaking, "Interface" isn't very meaningful in a type name. https://codereview.chromium.org/2565323002/diff/20001/base/memory/memory_coor... base/memory/memory_coordinator_proxy.h:32: void Set(WeakPtr<MemoryCoordinatorInterface> instance); On 2016/12/12 07:38:39, bashi1 wrote: > On 2016/12/12 07:27:07, dcheng wrote: > > Does this need to be a weak pointer? Maybe it should just unregister itself on > > destruction by calling this with nullptr? > > Sure. But I think it would be better to have explicit logic to avoid > use-after-free rather than putting comments like "must be removed on > destruction". WDYT? I actually prefer that model: the WeakPtr make it seem like we might have to dispatch calls on a potentially deleted object, when we can accurately manage lifetimes ourselves. I would just say that MemoryCoordinatorInterface implementations should just self-unregister in their destructor. However it looks like MemoryCoordinatorInterface is a leaky singleton, so it will never be destroyed anyway?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
LGTM
On 2016/12/12 08:31:13, dcheng wrote: > https://codereview.chromium.org/2565323002/diff/20001/base/memory/memory_coor... > File base/memory/memory_coordinator_proxy.h (right): > > https://codereview.chromium.org/2565323002/diff/20001/base/memory/memory_coor... > base/memory/memory_coordinator_proxy.h:21: virtual MemoryState > GetLocalMemoryState() = 0; > On 2016/12/12 07:38:39, bashi1 wrote: > > On 2016/12/12 07:27:07, dcheng wrote: > > > Two nits: > > > 1) I think it's more common to document the interface and not document the > > > implementation. Should we do that here? > > Let me clarify your comments. Do you mean moving comments from Proxy to here? > > Yes. > > > > > > > 2) It's not very common to have "Interface" in the names of interfaces. I > > think > > > it's more common to just have the interface be called "MemoryCoordinator" > and > > > have the implementation suffixed with Impl so it's called > > MemoryCoordinatorImpl. > > > > Yeah, but I can find such naming[1]. I totally agree with your comments but we > > already have MemoryCoordinator and MemoryCoordinatorImpl in content/. Do you > > want us to rename them before this CL? What kind of names we should in that > > case? > > > > [1] > > > https://cs.chromium.org/chromium/src/chrome/browser/task_manager/task_manager... > > If it would be hard for this CL, it's OK to defer. But I would like to think > about improving the names in a followup patch: generally speaking, "Interface" > isn't very meaningful in a type name. I'll try to rename them before this CL. > > https://codereview.chromium.org/2565323002/diff/20001/base/memory/memory_coor... > base/memory/memory_coordinator_proxy.h:32: void > Set(WeakPtr<MemoryCoordinatorInterface> instance); > On 2016/12/12 07:38:39, bashi1 wrote: > > On 2016/12/12 07:27:07, dcheng wrote: > > > Does this need to be a weak pointer? Maybe it should just unregister itself > on > > > destruction by calling this with nullptr? > > > > Sure. But I think it would be better to have explicit logic to avoid > > use-after-free rather than putting comments like "must be removed on > > destruction". WDYT? > > I actually prefer that model: the WeakPtr make it seem like we might have to > dispatch calls on a potentially deleted object, when we can accurately manage > lifetimes ourselves. Ok, I'll stop using WeakPtr. > > I would just say that MemoryCoordinatorInterface implementations should just > self-unregister in their destructor. However it looks like > MemoryCoordinatorInterface is a leaky singleton, so it will never be destroyed > anyway?
https://codereview.chromium.org/2565323002/diff/20001/base/memory/memory_coor... File base/memory/memory_coordinator_proxy.h (right): https://codereview.chromium.org/2565323002/diff/20001/base/memory/memory_coor... base/memory/memory_coordinator_proxy.h:21: virtual MemoryState GetLocalMemoryState() = 0; On 2016/12/12 08:31:13, dcheng wrote: > On 2016/12/12 07:38:39, bashi1 wrote: > > On 2016/12/12 07:27:07, dcheng wrote: > > > Two nits: > > > 1) I think it's more common to document the interface and not document the > > > implementation. Should we do that here? > > Let me clarify your comments. Do you mean moving comments from Proxy to here? > > Yes. > > > > > > > 2) It's not very common to have "Interface" in the names of interfaces. I > > think > > > it's more common to just have the interface be called "MemoryCoordinator" > and > > > have the implementation suffixed with Impl so it's called > > MemoryCoordinatorImpl. > > > > Yeah, but I can find such naming[1]. I totally agree with your comments but we > > already have MemoryCoordinator and MemoryCoordinatorImpl in content/. Do you > > want us to rename them before this CL? What kind of names we should in that > > case? > > > > [1] > > > https://cs.chromium.org/chromium/src/chrome/browser/task_manager/task_manager... > > If it would be hard for this CL, it's OK to defer. But I would like to think > about improving the names in a followup patch: generally speaking, "Interface" > isn't very meaningful in a type name. +1 to dropping "Interface" and putting the comments on the class defining the interface. https://codereview.chromium.org/2565323002/diff/20001/base/memory/memory_coor... base/memory/memory_coordinator_proxy.h:32: void Set(WeakPtr<MemoryCoordinatorInterface> instance); On 2016/12/12 08:31:13, dcheng wrote: > On 2016/12/12 07:38:39, bashi1 wrote: > > On 2016/12/12 07:27:07, dcheng wrote: > > > Does this need to be a weak pointer? Maybe it should just unregister itself > on > > > destruction by calling this with nullptr? > > > > Sure. But I think it would be better to have explicit logic to avoid > > use-after-free rather than putting comments like "must be removed on > > destruction". WDYT? > > I actually prefer that model: the WeakPtr make it seem like we might have to > dispatch calls on a potentially deleted object, when we can accurately manage > lifetimes ourselves. > > I would just say that MemoryCoordinatorInterface implementations should just > self-unregister in their destructor. However it looks like > MemoryCoordinatorInterface is a leaky singleton, so it will never be destroyed > anyway? Ditto. I'd prefer explicit management here. We already have lifetime issues in that GetInstance() returns a raw pointer, which a client may cache even after the WeakPtr is invalidated. https://codereview.chromium.org/2565323002/diff/20001/base/memory/memory_coor... base/memory/memory_coordinator_proxy.h:40: void SetMemoryStateForTesting(MemoryState memory_state); Why are these two functions needed here, if it's available on the Interface? This class should simply by the injection point, and the place where a client can get a pointer to a MemoryCoordinator, no? https://codereview.chromium.org/2565323002/diff/20001/content/browser/memory/... File content/browser/memory/memory_coordinator.h (right): https://codereview.chromium.org/2565323002/diff/20001/content/browser/memory/... content/browser/memory/memory_coordinator.h:63: virtual base::WeakPtr<base::MemoryCoordinatorInterface> GetWeakPtr(); Maybe call this GetBaseMemoryCoordinator? GetWeakPtr would tend to imply that the type is WeakPtr<MemoryCoordinator>... https://codereview.chromium.org/2565323002/diff/20001/content/browser/memory/... File content/browser/memory/memory_coordinator_impl.h (right): https://codereview.chromium.org/2565323002/diff/20001/content/browser/memory/... content/browser/memory/memory_coordinator_impl.h:42: public base::MemoryCoordinatorInterface, I'm not sure we need multiple inheritance here. Rather, I'd define a private MemoryCoordinatorInterface implementation in the .cc file, that wraps the necessary functions, and forwards them to the MemoryCoordinatorImpl? Then an accessor to get a pointer to that particular interface? This is the most flexible in the long run, as MemoryCoordinator and base::MemoryCoordinatorInterface end up having functions with the same name at some point...
Thanks Chris for review! I'm renaming content::MemoryCoordinator and content::MemoryCoordinatorImpl, and after that I'll revise this CL and address your comments (or I may upload a different CL and close this CL). |
