Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2086833005:
align WebMemoryPressureLevel with MemoryPressureListener::MemoryPressureLevel (Closed)
|
Created:
4 years, 6 months ago by hong.zheng Modified:
4 years, 5 months ago CC:
blink-reviews, blink-reviews-api_chromium.org, chromium-reviews, dglazkov+blink Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptionalign WebMemoryPressureLevel with MemoryPressureListener::MemoryPressureLevel
MemoryPressureListener::MemoryPressureLevel enum has changed to 0, 1, 2.
Let's align WebMemoryPressureLevel with it
BUG=
Committed: https://crrev.com/2e296f823ae220a5eae4ce86a8a34d3a190a6429
Cr-Commit-Position: refs/heads/master@{#402691}
Patch Set 1 #Patch Set 2 : add static_assert check #
Total comments: 1
Patch Set 3 : check in render_thread_impl.cc #
Messages
Total messages: 29 (9 generated)
hong.zheng@intel.com changed reviewers: + tkent@chromium.org
PTAL
tkent@chromium.org changed reviewers: + bashi@chromium.org
+bashi It's unfortunate that we can't prevent such mismatch. Can you add static_assert to check this to render_thread_impl.cc, which casts WebMemoryPressureLevel to MemoryPressureLevel.
Thanks for fixing this! We are replacing memory pressure notification with memory coordinator (e.g. https://codereview.chromium.org/2094583002/) but it will take time so we definitely need a static_assert.
Thanks for your comments. Add static_assert check. PTAL
https://codereview.chromium.org/2086833005/diff/20001/third_party/WebKit/publ... File third_party/WebKit/public/platform/WebMemoryPressureLevel.h (right): https://codereview.chromium.org/2086833005/diff/20001/third_party/WebKit/publ... third_party/WebKit/public/platform/WebMemoryPressureLevel.h:20: static_assert(static_cast<WebMemoryPressureLevel>( Can you put these static aseerts in RenderThreadImpl::OnMemoryPressure() ? I don't think we can include base/memory/memory_pressure_listener.h from WebKit/public/platform.
On 2016/06/23 05:07:37, bashi1 wrote: > https://codereview.chromium.org/2086833005/diff/20001/third_party/WebKit/publ... > File third_party/WebKit/public/platform/WebMemoryPressureLevel.h (right): > > https://codereview.chromium.org/2086833005/diff/20001/third_party/WebKit/publ... > third_party/WebKit/public/platform/WebMemoryPressureLevel.h:20: > static_assert(static_cast<WebMemoryPressureLevel>( > Can you put these static aseerts in RenderThreadImpl::OnMemoryPressure() ? > > I don't think we can include base/memory/memory_pressure_listener.h from > WebKit/public/platform. There is V8 static_assert check in the front part of render_thread_impl.cc. So I also add WebMemoryPressureLevel check like V8. Is it ok?
> There is V8 static_assert check in the front part of render_thread_impl.cc. So I also add WebMemoryPressureLevel check like V8. Is it ok? It makes sense perfectly. LGTM.
hong.zheng@intel.com changed reviewers: + esprehn@chromium.org
Thanks. esprehn, could you please review render_thread_impl.cc part?
non-owner lgtm
The CQ bit was checked by hong.zheng@intel.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2086833005/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_compile_dbg_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
ping esprehn, could you please take a look?
Why is everything red?
Seems that presubmit errors and compile errors w/o this CL. I'll kick a dry-run.
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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
lgtm
The CQ bit was checked by hong.zheng@intel.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== align WebMemoryPressureLevel with MemoryPressureListener::MemoryPressureLevel MemoryPressureListener::MemoryPressureLevel enum has changed to 0, 1, 2. Let's align WebMemoryPressureLevel with it BUG= ========== to ========== align WebMemoryPressureLevel with MemoryPressureListener::MemoryPressureLevel MemoryPressureListener::MemoryPressureLevel enum has changed to 0, 1, 2. Let's align WebMemoryPressureLevel with it BUG= Committed: https://crrev.com/2e296f823ae220a5eae4ce86a8a34d3a190a6429 Cr-Commit-Position: refs/heads/master@{#402691} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/2e296f823ae220a5eae4ce86a8a34d3a190a6429 Cr-Commit-Position: refs/heads/master@{#402691} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
