Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2720273003:
[Media Router] Remove binary messaging implementation. (Closed)
|
Created:
3 years, 9 months ago by mark a. foltz Modified:
3 years, 9 months ago CC:
agrieve+watch_chromium.org, chfremer+watch_chromium.org, chromium-reviews, feature-media-reviews_chromium.org, mcasas+watch+vc_chromium.org, media-router+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Media Router] Remove binary messaging implementation.
This removes the binary messaging implementation for Media Router from Android,
as there are no plans to support it for Cast.
BUG=524128
R=avayvod@chromium.org
Review-Url: https://codereview.chromium.org/2720273003 .
Cr-Original-Commit-Position: refs/heads/master@{#453709}
Committed: https://chromium.googlesource.com/chromium/src/+/ebdab4190ceb21a9ba4a5d8e719df9ab94b2716e
Review-Url: https://codereview.chromium.org/2720273003
Cr-Commit-Position: refs/heads/master@{#454364}
Committed: https://chromium.googlesource.com/chromium/src/+/e506b1518fc4633aee4f2b32193de02f987a0096
Patch Set 1 #Patch Set 2 : Revert of revert #Patch Set 3 : Fix MockMediaRouteProvider #Patch Set 4 : Apply suggested patch #
Messages
Total messages: 35 (17 generated)
mfoltz@chromium.org changed reviewers: + avayvod@chromium.org
avayvod@: Do you know why I am getting the following error? Imports look OK. ** Presubmit ERRORS ** chrome/android/java/src/org/chromium/chrome/browser/media/router/cast/CastMediaRouteProvider.java:20: Wrong order for org.json.JSONException import. Use :JavaImportOrganize (ECLIM), Ctrl+Shift+O (Eclipse), or Ctrl+Alt+O (Android Studio) to sort imports. An importorder file for configuring Eclipse can be found at //tools/android/eclipse/android.importorder. A style configuration file for Android Studio can be found at //tools/android/android_studio/ChromiumStyle.xml.
The CQ bit was checked by mfoltz@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...
lgtm I think there should be a blank line between org.chromium.* and org.json.* however I was sure presubmit is smart enough to check for modified lines only. Feel free to either fix or ignore :)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
Description was changed from ========== [Media Router] Remove binary messaging implementation. This removes the binary messaging implementation for Media Router from Android, as there are no plans to support it for Cast. BUG=524128 ========== to ========== [Media Router] Remove binary messaging implementation. This removes the binary messaging implementation for Media Router from Android, as there are no plans to support it for Cast. BUG=524128 R=avayvod@chromium.org Review-Url: https://codereview.chromium.org/2720273003 . Cr-Commit-Position: refs/heads/master@{#453709} Committed: https://chromium.googlesource.com/chromium/src/+/ebdab4190ceb21a9ba4a5d8e719d... ==========
Message was sent while issue was closed.
Committed patchset #1 (id:1) manually as ebdab4190ceb21a9ba4a5d8e719df9ab94b2716e.
Message was sent while issue was closed.
A revert of this CL (patchset #1 id:1) has been created in https://codereview.chromium.org/2720363002/ by jam@chromium.org. The reason for reverting is: Breaks the build..
Message was sent while issue was closed.
A revert of this CL (patchset #1 id:1) has been created in https://codereview.chromium.org/2718073005/ by mfoltz@chromium.org. The reason for reverting is: Broke Android unittests.
The CQ bit was checked by mfoltz@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...
FYI: Findit identified this CL at revision 453709 as the culprit for failures in the build cycles as shown on: https://findit-for-me.appspot.com/waterfall/culprit?key=ag9zfmZpbmRpdC1mb3Itb...
PTAL at revised patch
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
lgtm
The CQ bit was checked by mfoltz@chromium.org
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
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
On 2017/03/01 at 18:19:17, commit-bot wrote: > Try jobs failed on following builders: > chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) I won't be able to submit through CQ without addressing the imports issue. avayvod@ do you have any contacts for the Java presubmit hooks who can help investigate?
mfoltz@chromium.org changed reviewers: + nyquist@chromium.org
nyquist@ can you PTAL and let me know what is wrong with the imports? 'git cl format' doesn't change the order in the patch.
On 2017/03/01 at 18:41:49, mark a. foltz wrote: > nyquist@ can you PTAL and let me know what is wrong with the imports? 'git cl format' doesn't change the order in the patch. Error in the previous comment: https://codereview.chromium.org/2720273003/#msg2
For style questions, a good starting point is here: https://chromium.googlesource.com/chromium/src/+/master/styleguide/styleguide.md For this specific question, you'd want the Java style guide, and for import order, we use the same as for Android code, so see their guide at http://source.android.com/source/code-style.html#order-import-statements If you want to see the configuration for our version of checkstyle, you can see here: https://chromium.googlesource.com/chromium/src/+/6e560d0309ec9a4016f07b1da296... That lists the order that you failed to comply with in your patch.
Oh, and to answer your question about what is wrong and how to fix it, apply this diff: https://gist.github.com/tommynyquist/25b2bd1a9557b6b5e07a848270f0bd20
The CQ bit was checked by mfoltz@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from avayvod@chromium.org Link to the patchset: https://codereview.chromium.org/2720273003/#ps60001 (title: "Apply suggested patch")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/03/02 at 01:21:08, nyquist wrote: > Oh, and to answer your question about what is wrong and how to fix it, apply this diff: > https://gist.github.com/tommynyquist/25b2bd1a9557b6b5e07a848270f0bd20 Thank you very much for the fix. The documentation you cited on android.com around imports (and I already read) is conflicts with the actual checkstyle implementation. To avoid confusion, it would be helpful to revise the Chromium styleguide to reflect Chromium's specific import requirements enforced by checkstyle.
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1488482800085000,
"parent_rev": "104f81845c1906da368d74c20e4b161a348c0567", "commit_rev":
"e506b1518fc4633aee4f2b32193de02f987a0096"}
Message was sent while issue was closed.
Description was changed from ========== [Media Router] Remove binary messaging implementation. This removes the binary messaging implementation for Media Router from Android, as there are no plans to support it for Cast. BUG=524128 R=avayvod@chromium.org Review-Url: https://codereview.chromium.org/2720273003 . Cr-Commit-Position: refs/heads/master@{#453709} Committed: https://chromium.googlesource.com/chromium/src/+/ebdab4190ceb21a9ba4a5d8e719d... ========== to ========== [Media Router] Remove binary messaging implementation. This removes the binary messaging implementation for Media Router from Android, as there are no plans to support it for Cast. BUG=524128 R=avayvod@chromium.org Review-Url: https://codereview.chromium.org/2720273003 . Cr-Original-Commit-Position: refs/heads/master@{#453709} Committed: https://chromium.googlesource.com/chromium/src/+/ebdab4190ceb21a9ba4a5d8e719d... Review-Url: https://codereview.chromium.org/2720273003 Cr-Commit-Position: refs/heads/master@{#454364} Committed: https://chromium.googlesource.com/chromium/src/+/e506b1518fc4633aee4f2b32193d... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/e506b1518fc4633aee4f2b32193d... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
