Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2475293003:
[RemotePlayback] No monitoring on low-end devices (Closed)
Description[RemotePlayback] No monitoring on low-end devices
BUG=659672
TEST=layout test with extra plumbing through Internals
Committed: https://crrev.com/2707b633861011503944b886e36e3708e39d2593
Cr-Commit-Position: refs/heads/master@{#435095}
Patch Set 1 #Patch Set 2 : Added a layout test #Patch Set 3 : Added the internals files #
Total comments: 5
Patch Set 4 : Rebase on MC change #
Total comments: 2
Patch Set 5 : Rebase #Patch Set 6 : Set isLowEndDevice to false when the test passes #Patch Set 7 : Rebased on the last CL for isLowEndDevice #
Total comments: 3
Patch Set 8 : Added s_isLowEndDevice #Patch Set 9 : Reverted to a local static bool #Patch Set 10 : Use blink::initialize #
Total comments: 2
Patch Set 11 : Moved to Platform::initialize() #Messages
Total messages: 78 (34 generated)
The CQ bit was checked by avayvod@chromium.org to run a CQ dry run
avayvod@chromium.org changed reviewers: + mlamouri@chromium.org
PTaL
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Could you check whether a setIsLowEndDeviceForTesting() could be added and use this from Internals.idl in order to write a LayoutTest?
Added a layout test
PTAL
Added the internals files
The CQ bit was checked by avayvod@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
avayvod@chromium.org changed reviewers: + zqzhang@chromium.org
Seems like Mounir forgot to lgtm this one pending added layout test.
haraken@chromium.org changed reviewers: + bashi@chromium.org, haraken@chromium.org
(I think this is a bug of the architecture, so I'm not sure if I should ask you to fix this but...) it looks strange that the isLowEndDevice mode is controlled by Oilpan's ProcessHeap. Also it looks strange that the mode is overridden in RemotePlayback. A right fix would be to move the isLowEndDevice function to platform/MemoryCoordinator.h. Then you can add setIsLowEndDeviceForTesting to platform/MemoryCoordinator.h... +bashi FYI
lgtm but I think you should either move the internals to change the value of ProcessHeap or apply the follow-up that haraken@ suggested in another CL. https://codereview.chromium.org/2475293003/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/media/remoteplayback/watch-availability-throws-low-end-device.html (right): https://codereview.chromium.org/2475293003/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/media/remoteplayback/watch-availability-throws-low-end-device.html:7: <script src="../media-file.js"></script> style nit: we don't add that many indentation on LayoutTests usually. https://codereview.chromium.org/2475293003/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/media/remoteplayback/watch-availability-throws-low-end-device.html:18: v.remote.watchAvailability(t.step_func(function() {})).then( Instead of "t.step_func(function() {})", should this be `t.unreached_func()`? https://codereview.chromium.org/2475293003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/remoteplayback/testing/InternalsRemotePlayback.idl (right): https://codereview.chromium.org/2475293003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/remoteplayback/testing/InternalsRemotePlayback.idl:6: void setIsLowEndDevice(RemotePlayback remote, boolean isLowEndDevice); I was actually expecting the internals to change the value from ProcessHeap directly. Though, maybe you could keep this and do what haraken@ suggested in a follow-up?
https://codereview.chromium.org/2475293003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/remoteplayback/testing/InternalsRemotePlayback.idl (right): https://codereview.chromium.org/2475293003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/remoteplayback/testing/InternalsRemotePlayback.idl:6: void setIsLowEndDevice(RemotePlayback remote, boolean isLowEndDevice); On 2016/11/12 20:53:33, mlamouri (slow) wrote: > I was actually expecting the internals to change the value from ProcessHeap > directly. Though, maybe you could keep this and do what haraken@ suggested in a > follow-up? Yeah, that's fine. But at least I want to move this API from InternalsRemotePlayback.idl to WindowInternals.idl, since the functionality is not a specific thing to RemotePlayback.
platform/MemoryCoordinator seems the best place to have IsLowEndDevice() and SetIsLowEndDeviceForTesting(). https://codereview.chromium.org/2475293003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/remoteplayback/testing/InternalsRemotePlayback.idl (right): https://codereview.chromium.org/2475293003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/remoteplayback/testing/InternalsRemotePlayback.idl:6: void setIsLowEndDevice(RemotePlayback remote, boolean isLowEndDevice); On 2016/11/13 05:56:31, haraken wrote: > Yeah, that's fine. But at least I want to move this API from > InternalsRemotePlayback.idl to WindowInternals.idl, since the functionality is > not a specific thing to RemotePlayback. > +1. As for place to add the setter, how about Internals.idl?
Ok. I'm inclined at moving the existing call to MemoryCoordinator in the first CL and then use it in this one. Would you prefer me to add Internals.idl setter in the former or the latter CL (will be used in the latter)? The reason I had to add InternalsRemotePlayback is that Internals.* can't depend on modules/. I don't want to land this CL as is only to remove most of it in the follow up. Adding modules/ specific Internals was a good exercise, I wish the earlier comments were more specific or I were more courageous about changing base Blink classes :) On Sun, 13 Nov 2016, 18:45 , <bashi@chromium.org> wrote: > platform/MemoryCoordinator seems the best place to have IsLowEndDevice() > and > SetIsLowEndDeviceForTesting(). > > > > > https://codereview.chromium.org/2475293003/diff/40001/third_party/WebKit/Sour... > File > > third_party/WebKit/Source/modules/remoteplayback/testing/InternalsRemotePlayback.idl > (right): > > > https://codereview.chromium.org/2475293003/diff/40001/third_party/WebKit/Sour... > > third_party/WebKit/Source/modules/remoteplayback/testing/InternalsRemotePlayback.idl:6: > void setIsLowEndDevice(RemotePlayback remote, boolean isLowEndDevice); > On 2016/11/13 05:56:31, haraken wrote: > > Yeah, that's fine. But at least I want to move this API from > > InternalsRemotePlayback.idl to WindowInternals.idl, since the > functionality is > > not a specific thing to RemotePlayback. > > > > +1. As for place to add the setter, how about Internals.idl? > > https://codereview.chromium.org/2475293003/ > -- You received this message because you are subscribed to the Google Groups "Blink Reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to blink-reviews+unsubscribe@chromium.org.
Ok. I'm inclined at moving the existing call to MemoryCoordinator in the first CL and then use it in this one. Would you prefer me to add Internals.idl setter in the former or the latter CL (will be used in the latter)? The reason I had to add InternalsRemotePlayback is that Internals.* can't depend on modules/. I don't want to land this CL as is only to remove most of it in the follow up. Adding modules/ specific Internals was a good exercise, I wish the earlier comments were more specific or I were more courageous about changing base Blink classes :) On Sun, 13 Nov 2016, 18:45 , <bashi@chromium.org> wrote: > platform/MemoryCoordinator seems the best place to have IsLowEndDevice() > and > SetIsLowEndDeviceForTesting(). > > > > > https://codereview.chromium.org/2475293003/diff/40001/third_party/WebKit/Sour... > File > > third_party/WebKit/Source/modules/remoteplayback/testing/InternalsRemotePlayback.idl > (right): > > > https://codereview.chromium.org/2475293003/diff/40001/third_party/WebKit/Sour... > > third_party/WebKit/Source/modules/remoteplayback/testing/InternalsRemotePlayback.idl:6: > void setIsLowEndDevice(RemotePlayback remote, boolean isLowEndDevice); > On 2016/11/13 05:56:31, haraken wrote: > > Yeah, that's fine. But at least I want to move this API from > > InternalsRemotePlayback.idl to WindowInternals.idl, since the > functionality is > > not a specific thing to RemotePlayback. > > > > +1. As for place to add the setter, how about Internals.idl? > > https://codereview.chromium.org/2475293003/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2016/11/14 01:14:08, whywhat wrote: > Ok. I'm inclined at moving the existing call to MemoryCoordinator in the > first CL and then use it in this one. Sounds great! > Would you prefer me to add > Internals.idl setter in the former or the latter CL (will be used in the > latter)? If it's used in the latter CL, the latter CL would make more sense.
Rebase on MC change
Description was changed from ========== [RemotePlayback] No monitoring on low-end devices BUG=659672 TEST=manual, haven't found a way to emulate low-end device in blink ========== to ========== [RemotePlayback] No monitoring on low-end devices BUG=659672 TEST=layout test with extra plumbing through Internals ==========
PTAL (depends on https://codereview.chromium.org/2503433003)
LGTM, thanks!
lgtm with question addressed. https://codereview.chromium.org/2475293003/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/media/remoteplayback/watch-availability-throws-low-end-device.html (right): https://codereview.chromium.org/2475293003/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/media/remoteplayback/watch-availability-throws-low-end-device.html:16: internals.setIsLowEndDevice(true); Would this leak to other tests?
Rebase
Set isLowEndDevice to false when the test passes
The CQ bit was checked by avayvod@chromium.org to run a CQ dry run
https://codereview.chromium.org/2475293003/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/media/remoteplayback/watch-availability-throws-low-end-device.html (right): https://codereview.chromium.org/2475293003/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/media/remoteplayback/watch-availability-throws-low-end-device.html:16: internals.setIsLowEndDevice(true); On 2016/11/15 at 18:18:32, mlamouri (slow) wrote: > Would this leak to other tests? I dunno :/ If we reuse the instance of the content_shell for numerous tests (which we seem to do at least locally), yes. How do other internals settings tackle this? Set it to false at the end of the test now.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/11/15 20:24:24, whywhat wrote: > https://codereview.chromium.org/2475293003/diff/60001/third_party/WebKit/Layo... > File > third_party/WebKit/LayoutTests/media/remoteplayback/watch-availability-throws-low-end-device.html > (right): > > https://codereview.chromium.org/2475293003/diff/60001/third_party/WebKit/Layo... > third_party/WebKit/LayoutTests/media/remoteplayback/watch-availability-throws-low-end-device.html:16: > internals.setIsLowEndDevice(true); > On 2016/11/15 at 18:18:32, mlamouri (slow) wrote: > > Would this leak to other tests? > > I dunno :/ If we reuse the instance of the content_shell for numerous tests > (which we seem to do at least locally), yes. How do other internals settings > tackle this? > > Set it to false at the end of the test now. Sounds reasonable.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
lgtm
Rebased on the last CL for isLowEndDevice
PTAL wrt the new MemoryCoordinator::isLowEndDevice implementation.
The CQ bit was checked by avayvod@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/2475293003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/MemoryCoordinator.cpp (right): https://codereview.chromium.org/2475293003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/MemoryCoordinator.cpp:18: static bool isLowEndDevice = ::base::SysInfo::IsLowEndDevice(); Can we make this a static member of MemoryCoordinator? MemoryCoordinator::s_isLowEndDevice.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Added s_isLowEndDevice
https://codereview.chromium.org/2475293003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/MemoryCoordinator.cpp (right): https://codereview.chromium.org/2475293003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/MemoryCoordinator.cpp:18: static bool isLowEndDevice = ::base::SysInfo::IsLowEndDevice(); On 2016/11/23 at 02:31:31, haraken wrote: > Can we make this a static member of MemoryCoordinator? > > MemoryCoordinator::s_isLowEndDevice. Yeah, I guess we could still initialize it with SysInfo::IsLowEndDevice(), right? If we have to wait until the first call to isLowEndDevice() though, we'd have to make s_isLowEndDevice an Optional or add an extra bool so the initialization on demand doesn't override the test setting.
https://codereview.chromium.org/2475293003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/MemoryCoordinator.cpp (right): https://codereview.chromium.org/2475293003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/MemoryCoordinator.cpp:18: static bool isLowEndDevice = ::base::SysInfo::IsLowEndDevice(); On 2016/11/23 at 03:57:01, whywhat wrote: > On 2016/11/23 at 02:31:31, haraken wrote: > > Can we make this a static member of MemoryCoordinator? > > > > MemoryCoordinator::s_isLowEndDevice. > > Yeah, I guess we could still initialize it with SysInfo::IsLowEndDevice(), right? > If we have to wait until the first call to isLowEndDevice() though, we'd have to make s_isLowEndDevice an Optional or add an extra bool so the initialization on demand doesn't override the test setting. Seems like the answer is unsurprisingly, no: "declaration requires a global constructor".
Reverted to a local static bool
The CQ bit was checked by avayvod@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org, mlamouri@chromium.org, bashi@chromium.org Link to the patchset: https://codereview.chromium.org/2475293003/#ps160001 (title: "Reverted to a local static bool")
The CQ bit was checked by avayvod@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...
FWIW, the value of SysInfo::IsLowEndDevice() may depend on when it is called. crbug.com/657939 We might want to just call SysInfo::IsLowEndDevice() in MemoryCoordinator::isLowEndDevice().
On 2016/11/23 05:15:27, bashi1 wrote: > FWIW, the value of SysInfo::IsLowEndDevice() may depend on when it is called. > > crbug.com/657939 > > We might want to just call SysInfo::IsLowEndDevice() in > MemoryCoordinator::isLowEndDevice(). Can the return value of SysInfo::IsLowEndDevice() change after blink::initialize() is called? I was thinking about initializing s_isLowEndDevice in blink::initialize().
On 2016/11/23 05:40:59, haraken wrote: > On 2016/11/23 05:15:27, bashi1 wrote: > > FWIW, the value of SysInfo::IsLowEndDevice() may depend on when it is called. > > > > crbug.com/657939 > > > > We might want to just call SysInfo::IsLowEndDevice() in > > MemoryCoordinator::isLowEndDevice(). > > Can the return value of SysInfo::IsLowEndDevice() change after > blink::initialize() is called? I was thinking about initializing > s_isLowEndDevice in blink::initialize(). I haven't checked but I think it would be unchanged after initialization. But I'm not sure why we want to cache the value which might get stale.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/11/23 05:55:06, bashi1 wrote: > On 2016/11/23 05:40:59, haraken wrote: > > On 2016/11/23 05:15:27, bashi1 wrote: > > > FWIW, the value of SysInfo::IsLowEndDevice() may depend on when it is > called. > > > > > > crbug.com/657939 > > > > > > We might want to just call SysInfo::IsLowEndDevice() in > > > MemoryCoordinator::isLowEndDevice(). > > > > Can the return value of SysInfo::IsLowEndDevice() change after > > blink::initialize() is called? I was thinking about initializing > > s_isLowEndDevice in blink::initialize(). > > I haven't checked but I think it would be unchanged after initialization. But > I'm not sure why we want to cache the value which might get stale. Yeah, agreed. There's no strong reason we have to cache the value. So, can we do: - Call base::SysInfo::IsLowEndDevice() always. - But make it overridable by MemoryCoordinator::s_isLowEndDevice ?
On 2016/11/23 at 06:17:26, haraken wrote:
> On 2016/11/23 05:55:06, bashi1 wrote:
> > On 2016/11/23 05:40:59, haraken wrote:
> > > On 2016/11/23 05:15:27, bashi1 wrote:
> > > > FWIW, the value of SysInfo::IsLowEndDevice() may depend on when it is
> > called.
> > > >
> > > > crbug.com/657939
> > > >
> > > > We might want to just call SysInfo::IsLowEndDevice() in
> > > > MemoryCoordinator::isLowEndDevice().
> > >
> > > Can the return value of SysInfo::IsLowEndDevice() change after
> > > blink::initialize() is called? I was thinking about initializing
> > > s_isLowEndDevice in blink::initialize().
> >
> > I haven't checked but I think it would be unchanged after initialization.
But
> > I'm not sure why we want to cache the value which might get stale.
>
> Yeah, agreed. There's no strong reason we have to cache the value.
>
> So, can we do:
>
> - Call base::SysInfo::IsLowEndDevice() always.
> - But make it overridable by MemoryCoordinator::s_isLowEndDevice
>
> ?
How would you implement a three-state value (not overridden, overridden to
false, overridden to true)? As I told before, bool s_isLowEndDevice will not be
enough, I'll have to decide if the value was set to anything before returning
base::SysInfo::IsLowEndDevice().
I guess having an enum like IsLowEndDeviceForTesting { Default, OverriddenTrue,
OverriddenFalse } could be the cleanest way.
Could've a second bool |isLowEndDeviceOverridden| or a bool* |isLowEndDevice|.
wtf::Optional will not probably work as a static as it's not a primitive type?
On 2016/11/23 18:16:50, whywhat_OOO_till_Mon_Nov_28 wrote:
> On 2016/11/23 at 06:17:26, haraken wrote:
> > On 2016/11/23 05:55:06, bashi1 wrote:
> > > On 2016/11/23 05:40:59, haraken wrote:
> > > > On 2016/11/23 05:15:27, bashi1 wrote:
> > > > > FWIW, the value of SysInfo::IsLowEndDevice() may depend on when it is
> > > called.
> > > > >
> > > > > crbug.com/657939
> > > > >
> > > > > We might want to just call SysInfo::IsLowEndDevice() in
> > > > > MemoryCoordinator::isLowEndDevice().
> > > >
> > > > Can the return value of SysInfo::IsLowEndDevice() change after
> > > > blink::initialize() is called? I was thinking about initializing
> > > > s_isLowEndDevice in blink::initialize().
> > >
> > > I haven't checked but I think it would be unchanged after initialization.
> But
> > > I'm not sure why we want to cache the value which might get stale.
> >
> > Yeah, agreed. There's no strong reason we have to cache the value.
> >
> > So, can we do:
> >
> > - Call base::SysInfo::IsLowEndDevice() always.
> > - But make it overridable by MemoryCoordinator::s_isLowEndDevice
> >
> > ?
>
> How would you implement a three-state value (not overridden, overridden to
> false, overridden to true)? As I told before, bool s_isLowEndDevice will not
be
> enough, I'll have to decide if the value was set to anything before returning
> base::SysInfo::IsLowEndDevice().
>
> I guess having an enum like IsLowEndDeviceForTesting { Default,
OverriddenTrue,
> OverriddenFalse } could be the cleanest way.
> Could've a second bool |isLowEndDeviceOverridden| or a bool* |isLowEndDevice|.
> wtf::Optional will not probably work as a static as it's not a primitive type?
Then an easier option would be:
- Initialize MemoryCoordinator::s_isLowEndDevice in blink::initialize() (I think
it's okay to assume that base::SysInfo::IsLowEndDevice() doesn't change after
blink::initialize())
- Overwrite MemoryCoordinator::s_isLowEndDevice in SetIsLowEndDeviceForTesting
?
Use blink::initialize
The CQ bit was checked by avayvod@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...
PTAL Using base::SysInfo is not allowed from WebKit.cpp so I had to add an initialize() method to MemoryCoordinator. Does blink::initialize() run in all cases where IsLowEndDevice() is called, e.g. for blink_heap_unittests and so on?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM with a comment. https://codereview.chromium.org/2475293003/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/WebKit.cpp (right): https://codereview.chromium.org/2475293003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/web/WebKit.cpp:86: MemoryCoordinator::initialize(); We should call this in Platform::initialize since MemoryCoordinator is a platform thing.
Moved to Platform::initialize()
https://codereview.chromium.org/2475293003/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/WebKit.cpp (right): https://codereview.chromium.org/2475293003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/web/WebKit.cpp:86: MemoryCoordinator::initialize(); On 2016/11/29 at 02:02:46, haraken wrote: > We should call this in Platform::initialize since MemoryCoordinator is a platform thing. Done.
The CQ bit was checked by avayvod@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bashi@chromium.org, mlamouri@chromium.org, haraken@chromium.org Link to the patchset: https://codereview.chromium.org/2475293003/#ps200001 (title: "Moved to Platform::initialize()")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 200001, "attempt_start_ts": 1480449899989700,
"parent_rev": "c2a0e06d725cd4825ffbfec821b1b9d55b262121", "commit_rev":
"e04e65ff2d485ce8498786891791837b1c29f934"}
Message was sent while issue was closed.
Committed patchset #11 (id:200001)
Message was sent while issue was closed.
Description was changed from ========== [RemotePlayback] No monitoring on low-end devices BUG=659672 TEST=layout test with extra plumbing through Internals ========== to ========== [RemotePlayback] No monitoring on low-end devices BUG=659672 TEST=layout test with extra plumbing through Internals Committed: https://crrev.com/2707b633861011503944b886e36e3708e39d2593 Cr-Commit-Position: refs/heads/master@{#435095} ==========
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/2707b633861011503944b886e36e3708e39d2593 Cr-Commit-Position: refs/heads/master@{#435095} |
