Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
12026010:
Add GOOGLE_TV build tag (Closed)
|
Created:
7 years, 11 months ago by kjyoun Modified:
7 years, 10 months ago CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, jam, googletv-nikita_google.com Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionAdd GOOGLE_TV build tag
add google_tv GYP variable for 'Android for Chrome' running on TV
BUG=172815
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=179798
Patch Set 1 #Patch Set 2 : removed garbage line #Patch Set 3 : remove webrtc to avoid build error #Patch Set 4 : add missing header #
Total comments: 8
Patch Set 5 : addressed comments #Patch Set 6 : rename android_tv to google_tv #Patch Set 7 : change commit message #
Total comments: 4
Patch Set 8 : filed crbug and updated todo #
Total comments: 6
Patch Set 9 : #
Total comments: 4
Patch Set 10 : move a flag for EME under clank #Patch Set 11 : rebased and add bug number #Patch Set 12 : move ENABLE_EME under chrome #Patch Set 13 : rebased and remove empry line #
Messages
Total messages: 25 (0 generated)
Can you start review this?
I assume there are more changes along these lines? Can you file a tracking bug along the lines of "Experiment with building for android tv"? https://codereview.chromium.org/12026010/diff/6001/build/common.gypi File build/common.gypi (right): https://codereview.chromium.org/12026010/diff/6001/build/common.gypi#newcode113 build/common.gypi:113: # Sets whehther chrome is built for android tv device Nit: add trailing ".". Fix type: "whether" https://codereview.chromium.org/12026010/diff/6001/build/common.gypi#newcode533 build/common.gypi:533: ['(OS=="anroid" and android_tv!=1) or OS=="ios"', { OS=="android".. https://codereview.chromium.org/12026010/diff/6001/content/browser/android/co... File content/browser/android/content_startup_flags.cc (right): https://codereview.chromium.org/12026010/diff/6001/content/browser/android/co... content/browser/android/content_startup_flags.cc:72: // Cdm plugin should run out of proces What's the "Cdm plugin"? Can you add a TODO and file a crbug
https://codereview.chromium.org/12026010/diff/6001/build/common.gypi File build/common.gypi (right): https://codereview.chromium.org/12026010/diff/6001/build/common.gypi#newcode113 build/common.gypi:113: # Sets whehther chrome is built for android tv device On 2013/01/23 00:45:28, Yaron wrote: > Nit: add trailing ".". Fix type: "whether" Done. https://codereview.chromium.org/12026010/diff/6001/build/common.gypi#newcode533 build/common.gypi:533: ['(OS=="anroid" and android_tv!=1) or OS=="ios"', { On 2013/01/23 00:45:28, Yaron wrote: > OS=="android".. Done. https://codereview.chromium.org/12026010/diff/6001/content/browser/android/co... File content/browser/android/content_startup_flags.cc (right): https://codereview.chromium.org/12026010/diff/6001/content/browser/android/co... content/browser/android/content_startup_flags.cc:72: // Cdm plugin should run out of proces Cdm plugin is a pepper plugin to support Encrypted Media Extensions by doing key exchange and decryption. Source coe of Cdm plugin itself will reside in GoogleTV source tree.
I changed android_tv to google_tv, per Robert's recommendation BTW, though I changed my commit page, the change to commit message and title was not reflected on Chrome review page. How can I change them on Chrome review age?
On 2013/01/23 23:33:39, kjyoun wrote: > I changed android_tv to google_tv, > per Robert's recommendation > > BTW, though I changed my commit page, > the change to commit message and title was not reflected on Chrome review page. > How can I change them on Chrome review age? You can change by going to 'edit issue' on chromiumcodereview (i.e. in the column over there <----) :)
https://chromiumcodereview.appspot.com/12026010/diff/12005/build/common.gypi File build/common.gypi (right): https://chromiumcodereview.appspot.com/12026010/diff/12005/build/common.gypi#... build/common.gypi:533: ['(OS=="android" and google_tv!=1) or OS=="ios"', { why not just google_tv!=1 or OS=="ios"
https://codereview.chromium.org/12026010/diff/6001/content/browser/android/co... File content/browser/android/content_startup_flags.cc (right): https://codereview.chromium.org/12026010/diff/6001/content/browser/android/co... content/browser/android/content_startup_flags.cc:72: // Cdm plugin should run out of proces On 2013/01/23 21:02:57, kjyoun wrote: > Cdm plugin is a pepper plugin to support Encrypted Media Extensions by doing key > exchange and decryption. Source coe of Cdm plugin itself will reside in GoogleTV > source tree. Please add a TODO and file a crbug as requested (pertaining to your comment of "Will remove...") https://codereview.chromium.org/12026010/diff/12005/build/common.gypi File build/common.gypi (right): https://codereview.chromium.org/12026010/diff/12005/build/common.gypi#newcode113 build/common.gypi:113: # Sets whether chrome is built for android tv device. "google" (i.e. be consistent)
https://codereview.chromium.org/12026010/diff/6001/content/browser/android/co... File content/browser/android/content_startup_flags.cc (right): https://codereview.chromium.org/12026010/diff/6001/content/browser/android/co... content/browser/android/content_startup_flags.cc:72: // Cdm plugin should run out of proces On 2013/01/24 00:32:29, Yaron wrote: > On 2013/01/23 21:02:57, kjyoun wrote: > > Cdm plugin is a pepper plugin to support Encrypted Media Extensions by doing > key > > exchange and decryption. Source coe of Cdm plugin itself will reside in > GoogleTV > > source tree. > > Please add a TODO and file a crbug as requested (pertaining to your comment of > "Will remove...") Done. https://codereview.chromium.org/12026010/diff/12005/build/common.gypi File build/common.gypi (right): https://codereview.chromium.org/12026010/diff/12005/build/common.gypi#newcode113 build/common.gypi:113: # Sets whether chrome is built for android tv device. On 2013/01/24 00:32:29, Yaron wrote: > "google" (i.e. be consistent) Done. https://codereview.chromium.org/12026010/diff/12005/build/common.gypi#newcode533 build/common.gypi:533: ['(OS=="android" and google_tv!=1) or OS=="ios"', { We need to check android device, since google_tv == 0 does not necessarily mean that it does not need plugin For example,desktop chrome will have google_tv = 0 (since 0 is default value), but it needs plugin. On 2013/01/23 23:56:27, iannucci wrote: > why not just > > google_tv!=1 or OS=="ios"
lgtm but you need a webkit/OWNER
On 2013/01/24 22:55:52, Yaron wrote: > lgtm but you need a webkit/OWNER Thanks for the review.
Hi Brett/Darin Can you review my changes?
https://codereview.chromium.org/12026010/diff/9003/content/browser/android/co... File content/browser/android/content_startup_flags.cc (right): https://codereview.chromium.org/12026010/diff/9003/content/browser/android/co... content/browser/android/content_startup_flags.cc:72: // Cdm plugin should run out of process. Indenting is off. https://codereview.chromium.org/12026010/diff/9003/content/browser/android/co... content/browser/android/content_startup_flags.cc:74: parsed_command_line->AppendSwitch(switches::kPpapiOutOfProcess); This is not the right way to force a random plugin to run out of process. If you're registering programatically, you should set the out of process flag on the pepper plugin info. If you're registering on the command line you should just pass this flag. https://codereview.chromium.org/12026010/diff/9003/webkit/glue/webpreferences.cc File webkit/glue/webpreferences.cc (right): https://codereview.chromium.org/12026010/diff/9003/webkit/glue/webpreferences... webkit/glue/webpreferences.cc:148: user_gesture_required_for_media_playback(false) It's not clear to me what this has to do with the patch description. Is there a cleaner way we can do this? Filling this file up with ifdefs seems like the wrong way to configure WebKit, and I think it's asafe to say the current android stuff in here is "wrong".
https://codereview.chromium.org/12026010/diff/9003/content/browser/android/co... File content/browser/android/content_startup_flags.cc (right): https://codereview.chromium.org/12026010/diff/9003/content/browser/android/co... content/browser/android/content_startup_flags.cc:72: // Cdm plugin should run out of process. On 2013/01/25 22:46:42, brettw wrote: > Indenting is off. Done. https://codereview.chromium.org/12026010/diff/9003/content/browser/android/co... content/browser/android/content_startup_flags.cc:74: parsed_command_line->AppendSwitch(switches::kPpapiOutOfProcess); On 2013/01/25 22:46:42, brettw wrote: > This is not the right way to force a random plugin to run out of process. If > you're registering programatically, you should set the out of process flag on > the pepper plugin info. If you're registering on the command line you should > just pass this flag. Agreed. will pass this info when we register plugin, which will be done on https://codereview.chromium.org/11421146/ https://codereview.chromium.org/12026010/diff/9003/webkit/glue/webpreferences.cc File webkit/glue/webpreferences.cc (right): https://codereview.chromium.org/12026010/diff/9003/webkit/glue/webpreferences... webkit/glue/webpreferences.cc:148: user_gesture_required_for_media_playback(false) Agreed that this part should be cleaner. To make this part clean, I might have to make more extensive change, which I will put as my TODO list and will submit separate CL.
https://codereview.chromium.org/12026010/diff/18001/content/browser/android/c... File content/browser/android/content_startup_flags.cc (right): https://codereview.chromium.org/12026010/diff/18001/content/browser/android/c... content/browser/android/content_startup_flags.cc:72: parsed_command_line->AppendSwitch(switches::kEnableEncryptedMedia); can you do this somewhere under src/chrome? it's not clear why the content layer should know about chrome's needs for google tv.
https://codereview.chromium.org/12026010/diff/18001/content/browser/android/c... File content/browser/android/content_startup_flags.cc (right): https://codereview.chromium.org/12026010/diff/18001/content/browser/android/c... content/browser/android/content_startup_flags.cc:72: parsed_command_line->AppendSwitch(switches::kEnableEncryptedMedia); Agreed, since this flag is googletv-specific. Then right location might be src/native/framework/chrome/chrome_main_delegate_app_android.cc, which define flags for clank @Yaron Can you comment on this?
https://codereview.chromium.org/12026010/diff/18001/content/browser/android/c... File content/browser/android/content_startup_flags.cc (right): https://codereview.chromium.org/12026010/diff/18001/content/browser/android/c... content/browser/android/content_startup_flags.cc:72: parsed_command_line->AppendSwitch(switches::kEnableEncryptedMedia); fixed typo: right location seems to be "src/clank/native/framework/chrome/chrome_main_delegate_app_android.cc"
LGTM from me, but please work out with John the best place for your media config flag before checkin.
https://codereview.chromium.org/12026010/diff/18001/content/browser/android/c... File content/browser/android/content_startup_flags.cc (right): https://codereview.chromium.org/12026010/diff/18001/content/browser/android/c... content/browser/android/content_startup_flags.cc:72: parsed_command_line->AppendSwitch(switches::kEnableEncryptedMedia); verified that the change on clank side can make same impact and uploaded CL in clank https://gerrit-int.chromium.org/#/c/31695/1
lgtm
Can you review the change? If, hopefully, there is no problem, can you merge this code?
On 2013/01/30 21:49:53, kjyoun wrote: > Can you review the change? > > If, hopefully, there is no problem, > can you merge this code? Oh, did you not have the "Commit" checkbox? I clicked it for you
Um.. I could not find "Commit" checkbox. Where is it supposed to be located? Thanks
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/kjyoun@google.com/12026010/22004
On 2013/01/30 23:26:29, I haz the power (commit-bot) wrote: > CQ is trying da patch. Follow status at > https://chromium-status.appspot.com/cq/kjyoun%40google.com/12026010/22004 Oh, I guess you need to be a committer. It would show in the blue area under the file list.
Message was sent while issue was closed.
Change committed as 179798 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
