|
|
Created:
4 years, 2 months ago by Ken Rockot(use gerrit already) Modified:
4 years, 2 months ago CC:
Aaron Boodman, abarth-chromium, ccameron, chromium-reviews, creis+watch_chromium.org, darin (slow to review), darin-cc_chromium.org, einbinder+watch-test-runner_chromium.org, jam, jochen+watch_chromium.org, mlamouri+watch-test-runner_chromium.org, mlamouri+watch-content_chromium.org, nasko+codewatch_chromium.org, qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMove ViewMsg_New to mojom
This is added to a new content::mojom::Renderer interface which
is intended to define any messages processed directly by
RenderThreadImpl.
Also moves ViewMsg_New_Params to a mojom struct and adds
native typemaps for some of its dependencies.
BUG=612500
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
Committed: https://crrev.com/067ca55f59d8fcc92db8d4eb3551ef31b7b43119
Cr-Commit-Position: refs/heads/master@{#422236}
Patch Set 1 #Patch Set 2 : . #Patch Set 3 : . #
Total comments: 2
Patch Set 4 : . #
Total comments: 4
Patch Set 5 : . #Patch Set 6 : . #
Messages
Total messages: 48 (37 generated)
Description was changed from ========== Move ViewMsg_New to mojom This is added to a new content::mojom::Renderer interface which is intended to define any messages processed directly by RenderThreadImpl. Also moves ViewMsg_New_Params to a mojom struct and adds native typemaps for some of its dependencies. BUG=612500 ========== to ========== Move ViewMsg_New to mojom This is added to a new content::mojom::Renderer interface which is intended to define any messages processed directly by RenderThreadImpl. Also moves ViewMsg_New_Params to a mojom struct and adds native typemaps for some of its dependencies. BUG=612500 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
The CQ bit was checked by rockot@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: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_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 rockot@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: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by rockot@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...
rockot@chromium.org changed reviewers: + dcheng@chromium.org, nick@chromium.org, sky@chromium.org
Another CL for your reviewing pleasure. Please take a look: dcheng@ for mojom and typemaps (mostly native ParamTraits mappings and awful build rules) nick@ for content/ sky@ for ui/ Thanks!
LGTM
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Exceeded global retry quota
lgtm https://codereview.chromium.org/2381493003/diff/40001/content/renderer/render... File content/renderer/render_thread_impl.cc (right): https://codereview.chromium.org/2381493003/diff/40001/content/renderer/render... content/renderer/render_thread_impl.cc:352: // we want. Is this race still actually possible?
The CQ bit was checked by rockot@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: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...) cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Ping dcheng@ - I still have some annoying dependency issue to work out for the typemaps, but otherwise this is good for review
https://codereview.chromium.org/2381493003/diff/40001/content/renderer/render... File content/renderer/render_thread_impl.cc (right): https://codereview.chromium.org/2381493003/diff/40001/content/renderer/render... content/renderer/render_thread_impl.cc:352: // we want. On 2016/09/29 at 16:49:32, ncarter wrote: > Is this race still actually possible? Yeah, since this pipe is not associated with the channel, there's no ordering guarantees on it.
LGTM +ccameron as FYI for the icc profile stuff https://codereview.chromium.org/2381493003/diff/60001/ipc/ipc.mojom File ipc/ipc.mojom (right): https://codereview.chromium.org/2381493003/diff/60001/ipc/ipc.mojom#newcode8 ipc/ipc.mojom:8: const int32 ROUTING_ID_NONE = -2; Should this be named kRoutingIdNone to reflect the conventions around naming constants? Or is SHOUTY_CASE explicitly preferred for Java/JS interop? https://codereview.chromium.org/2381493003/diff/60001/ui/gfx/mojo/icc_profile... File ui/gfx/mojo/icc_profile_struct_traits.h (right): https://codereview.chromium.org/2381493003/diff/60001/ui/gfx/mojo/icc_profile... ui/gfx/mojo/icc_profile_struct_traits.h:28: std::unique_ptr<std::vector<int8_t>> data = Nit: out of line SetUpContext / TearDownContext.
The CQ bit was checked by rockot@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: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by rockot@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: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by rockot@chromium.org to run a CQ dry run
Patchset #6 (id:100001) has been deleted
Patchset #5 (id:80001) has been deleted
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/2381493003/diff/60001/ipc/ipc.mojom File ipc/ipc.mojom (right): https://codereview.chromium.org/2381493003/diff/60001/ipc/ipc.mojom#newcode8 ipc/ipc.mojom:8: const int32 ROUTING_ID_NONE = -2; On 2016/09/30 at 06:30:37, dcheng wrote: > Should this be named kRoutingIdNone to reflect the conventions around naming constants? Or is SHOUTY_CASE explicitly preferred for Java/JS interop? Done. I think I was just tricked by the definition of MSG_ROUTING_NONE. https://codereview.chromium.org/2381493003/diff/60001/ui/gfx/mojo/icc_profile... File ui/gfx/mojo/icc_profile_struct_traits.h (right): https://codereview.chromium.org/2381493003/diff/60001/ui/gfx/mojo/icc_profile... ui/gfx/mojo/icc_profile_struct_traits.h:28: std::unique_ptr<std::vector<int8_t>> data = On 2016/09/30 at 06:30:37, dcheng wrote: > Nit: out of line SetUpContext / TearDownContext. Better: I removed these and eliminated the extra copying. Per our off-thread discussion I attempted to change ICCProfile to use uint8_t for storage, but it turns out that the vector<char> gets passed around all over the place and the CL started to explode in size. Changed the mojom type to use a string instead, and conveniently the traits can just return a base::StringPiece over the internal vector<char>.
The CQ bit was checked by rockot@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...
mojo still lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by rockot@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from nick@chromium.org, sky@chromium.org Link to the patchset: https://codereview.chromium.org/2381493003/#ps140001 (title: ".")
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 #6 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== Move ViewMsg_New to mojom This is added to a new content::mojom::Renderer interface which is intended to define any messages processed directly by RenderThreadImpl. Also moves ViewMsg_New_Params to a mojom struct and adds native typemaps for some of its dependencies. BUG=612500 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Move ViewMsg_New to mojom This is added to a new content::mojom::Renderer interface which is intended to define any messages processed directly by RenderThreadImpl. Also moves ViewMsg_New_Params to a mojom struct and adds native typemaps for some of its dependencies. BUG=612500 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation Committed: https://crrev.com/067ca55f59d8fcc92db8d4eb3551ef31b7b43119 Cr-Commit-Position: refs/heads/master@{#422236} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/067ca55f59d8fcc92db8d4eb3551ef31b7b43119 Cr-Commit-Position: refs/heads/master@{#422236} |