Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1478633002:
Removing references to //ppapi when is_android = true (Closed)
|
Created:
5 years ago by agrieve Modified:
5 years ago CC:
chromium-reviews, mlamouri+watch-content_chromium.org, vabr+watchlistpasswordmanager_chromium.org, Peter Beverloo, jam, darin-cc_chromium.org, mkwst+moarreviews-renderer_chromium.org, gcasto+watchlist_chromium.org, mkwst+moarreviews-shell_chromium.org, jochen+watch_chromium.org, mkwst+watchlist-passwords_chromium.org, alokp Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionGN: Removing references to //ppapi when !enable_plugins
BUG=504082
Committed: https://crrev.com/e786dac49103ead759361788d366d62f5a42c58a
Cr-Commit-Position: refs/heads/master@{#362703}
Patch Set 1 #Patch Set 2 : is_android -> enable_plugins #Patch Set 3 : make assert android-only #
Total comments: 6
Patch Set 4 : review comments #Patch Set 5 : fix analyze failure #
Messages
Total messages: 41 (18 generated)
agrieve@chromium.org changed reviewers: + yfriedman@chromium.org
Yaron - please check that this makes sense.
yfriedman@chromium.org changed reviewers: + mfomitchev@chromium.org
+mfomitchev - I think this may be needed for android + aura
I think people want to support PPAPI on Android Aura..
On 2015/11/25 15:49:00, Yaron wrote: > +mfomitchev - I think this may be needed for android + aura Yeah, was also wondering which of these should actually be behind "if (enable_plugins)" rather than is_android.
On 2015/11/25 16:54:12, agrieve wrote: > On 2015/11/25 15:49:00, Yaron wrote: > > +mfomitchev - I think this may be needed for android + aura > > Yeah, was also wondering which of these should actually be behind "if > (enable_plugins)" rather than is_android. Sure, if you use enable_plugins, it would work for Android Aura. Currently enable_plugins = (!is_android && !is_ios) || is_chromecast So you'd be enabling it for chromecast too - I am not sure if that's a good thing?
The CQ bit was checked by agrieve@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1478633002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1478633002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
The CQ bit was checked by agrieve@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1478633002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1478633002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2015/11/26 05:40:12, commit-bot: I haz the power wrote: > Dry run: This issue passed the CQ dry run. Switched to using enable_plugins.
yfriedman@chromium.org changed reviewers: + gunsch@chromium.org
+gunsch who can hopefully comment on the chromecast aspect lgtm if he's ok with it
lgtm from chromecast side, but +cc alokp as FYI chromecast linux enables plugins, chromecast android (e.g. for Android TV) does not
Description was changed from ========== Removing references to //ppapi when is_android = true BUG=504082 ========== to ========== GN: Removing references to //ppapi when is_android = true BUG=504082 ==========
Description was changed from ========== GN: Removing references to //ppapi when is_android = true BUG=504082 ========== to ========== GN: Removing references to //ppapi when !enable_plugins BUG=504082 ==========
agrieve@chromium.org changed reviewers: + brettw@chromium.org
brettw for OWNERS ᶘᵒᴥᵒᶅ
https://codereview.chromium.org/1478633002/diff/40001/components/BUILD.gn File components/BUILD.gn (right): https://codereview.chromium.org/1478633002/diff/40001/components/BUILD.gn#new... components/BUILD.gn:672: "//ipc:test_support", I don't see what this is for. Did something fail without it? https://codereview.chromium.org/1478633002/diff/40001/content/shell/renderer/... File content/shell/renderer/shell_content_renderer_client.cc (right): https://codereview.chromium.org/1478633002/diff/40001/content/shell/renderer/... content/shell/renderer/shell_content_renderer_client.cc:11: #if defined(ENABLE_PLUGINS) Put conditional includes after the rest of the includes with a blank between the two blocks. https://codereview.chromium.org/1478633002/diff/40001/content/test/BUILD.gn File content/test/BUILD.gn (right): https://codereview.chromium.org/1478633002/diff/40001/content/test/BUILD.gn#n... content/test/BUILD.gn:90: if (enable_plugins) { I think this block should be moved out of the iOS block. It doesn't actually have anything to do with iOS (or not), and iOS should always have plugins set to disabled.
https://codereview.chromium.org/1478633002/diff/40001/components/BUILD.gn File components/BUILD.gn (right): https://codereview.chromium.org/1478633002/diff/40001/components/BUILD.gn#new... components/BUILD.gn:672: "//ipc:test_support", On 2015/12/01 18:01:37, brettw wrote: > I don't see what this is for. Did something fail without it? Without this: ../../components/password_manager/content/renderer/credential_manager_client_browsertest.cc:51: error: undefined reference to 'IPC::TestSink::GetFirstMessageMatching(unsigned int) const' ../../components/password_manager/content/renderer/credential_manager_client_browsertest.cc:80: error: undefined reference to 'IPC::TestSink::ClearMessages()' ../../components/tracing/child_trace_message_filter_browsertest.cc:184: error: undefined reference to 'IPC::TestSink::ClearMessages()' ../../components/tracing/child_trace_message_filter_browsertest.cc:203: error: undefined reference to 'IPC::TestSink::ClearMessages()' ../../components/tracing/child_trace_message_filter_browsertest.cc:157: error: undefined reference to 'IPC::TestSink::GetUniqueMessageMatching(unsigned int) const' ../../components/tracing/child_trace_message_filter_browsertest.cc:234: error: undefined reference to 'IPC::TestSink::ClearMessages()' ../../components/tracing/child_trace_message_filter_browsertest.cc:263: error: undefined reference to 'IPC::TestSink::GetUniqueMessageMatching(unsigned int) const' ../../components/tracing/child_trace_message_filter_browsertest.cc:71: error: undefined reference to 'IPC::TestSink::AddFilter(IPC::Listener*)' ../../components/tracing/child_trace_message_filter_browsertest.cc:79: error: undefined reference to 'IPC::TestSink::RemoveFilter(IPC::Listener*)' ../../components/tracing/child_trace_message_filter_browsertest.cc:105: error: undefined reference to 'IPC::TestSink::GetUniqueMessageMatching(unsigned int) const' ../../components/printing/test/print_web_view_helper_browsertest.cc:196: error: undefined reference to 'IPC::TestSink::GetUniqueMessageMatching(unsigned int) const' ../../components/printing/test/print_web_view_helper_browsertest.cc:227: error: undefined reference to 'IPC::TestSink::AddFilter(IPC::Listener*)' ../../components/printing/test/print_web_view_helper_browsertest.cc:230: error: undefined reference to 'IPC::TestSink::RemoveFilter(IPC::Listener*)' ../../components/printing/test/print_web_view_helper_browsertest.cc:557: error: undefined reference to 'IPC::TestSink::GetMessageAt(unsigned int) const' ../../content/public/test/mock_render_thread.cc:21: error: undefined reference to 'IPC::TestSink::TestSink()' ../../content/public/test/mock_render_thread.cc:21: error: undefined reference to 'IPC::TestSink::TestSink()' ../../content/public/test/mock_render_thread.cc:35: error: undefined reference to 'IPC::TestSink::~TestSink()' ../../content/public/test/mock_render_thread.cc:241: error: undefined reference to 'IPC::TestSink::OnMessageReceived(IPC::Message const&)' clang: error: linker command failed with exit code 1 (use -v to see invocation) https://codereview.chromium.org/1478633002/diff/40001/content/shell/renderer/... File content/shell/renderer/shell_content_renderer_client.cc (right): https://codereview.chromium.org/1478633002/diff/40001/content/shell/renderer/... content/shell/renderer/shell_content_renderer_client.cc:11: #if defined(ENABLE_PLUGINS) On 2015/12/01 18:01:37, brettw wrote: > Put conditional includes after the rest of the includes with a blank between the > two blocks. Done. https://codereview.chromium.org/1478633002/diff/40001/content/test/BUILD.gn File content/test/BUILD.gn (right): https://codereview.chromium.org/1478633002/diff/40001/content/test/BUILD.gn#n... content/test/BUILD.gn:90: if (enable_plugins) { On 2015/12/01 18:01:37, brettw wrote: > I think this block should be moved out of the iOS block. It doesn't actually > have anything to do with iOS (or not), and iOS should always have plugins set to > disabled. Re-jiggerred these conditions to make the is_ios part as small as possible.
lgtm
The CQ bit was checked by agrieve@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1478633002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1478633002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win8_chromium_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_ng/...)
The CQ bit was checked by agrieve@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1478633002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1478633002/80001
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 agrieve@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from gunsch@chromium.org, yfriedman@chromium.org, brettw@chromium.org Link to the patchset: https://codereview.chromium.org/1478633002/#ps80001 (title: "fix analyze failure")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1478633002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1478633002/80001
Message was sent while issue was closed.
Description was changed from ========== GN: Removing references to //ppapi when !enable_plugins BUG=504082 ========== to ========== GN: Removing references to //ppapi when !enable_plugins BUG=504082 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== GN: Removing references to //ppapi when !enable_plugins BUG=504082 ========== to ========== GN: Removing references to //ppapi when !enable_plugins BUG=504082 Committed: https://crrev.com/e786dac49103ead759361788d366d62f5a42c58a Cr-Commit-Position: refs/heads/master@{#362703} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/e786dac49103ead759361788d366d62f5a42c58a Cr-Commit-Position: refs/heads/master@{#362703} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
