Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1224643008:
Add a fake cast extension for testing. (Closed)
|
Created:
5 years, 5 months ago by jdufault Modified:
5 years, 5 months ago CC:
chromium-reviews, chromium-apps-reviews_chromium.org, extensions-reviews_chromium.org, miu+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@lkgr Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd a fake cast extension for testing.
The extension provides only the API needed by the ChromeOS system tray cast integration. It also provides a few utility methods that simplify the testing code.
This extension is used in https://codereview.chromium.org/1231593002/.
BUG=497343
Committed: https://crrev.com/2eb5b206baa2a20876658ed1d8e453f6c8a21a80
Cr-Commit-Position: refs/heads/master@{#339900}
Patch Set 1 #
Total comments: 4
Patch Set 2 : #
Total comments: 10
Patch Set 3 : #
Total comments: 1
Patch Set 4 : add comment before landing #Patch Set 5 : Add license #
Messages
Total messages: 28 (9 generated)
jdufault@chromium.org changed reviewers: + achuith@chromium.org
On 2015/07/08 18:01:49, jdufault wrote: > mailto:jdufault@chromium.org changed reviewers: > + mailto:achuith@chromium.org Achuith, would you mind taking a look?
https://codereview.chromium.org/1224643008/diff/1/chrome/browser/extensions/a... File chrome/browser/extensions/api/tab_capture/tab_capture_api.cc (right): https://codereview.chromium.org/1224643008/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/tab_capture/tab_capture_api.cc:69: "hlgmmjhlnlapooncikdpiiokdjcdpjme", Personally I think a comment like // Test cast extension is sufficient, but up to you https://codereview.chromium.org/1224643008/diff/1/chrome/test/data/extensions... File chrome/test/data/extensions/tray_cast/manifest.json (right): https://codereview.chromium.org/1224643008/diff/1/chrome/test/data/extensions... chrome/test/data/extensions/tray_cast/manifest.json:1: { Add a comment like: // chrome-extension://hlgmmjhlnlapooncikdpiiokdjcdpjme/ This way this file will show up in searches for this string
https://codereview.chromium.org/1224643008/diff/1/chrome/browser/extensions/a... File chrome/browser/extensions/api/tab_capture/tab_capture_api.cc (right): https://codereview.chromium.org/1224643008/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/tab_capture/tab_capture_api.cc:69: "hlgmmjhlnlapooncikdpiiokdjcdpjme", On 2015/07/08 22:23:55, achuithb wrote: > Personally I think a comment like > // Test cast extension > is sufficient, but up to you Done, using the string to search instead of hardcoding a path sounds much better to me. https://codereview.chromium.org/1224643008/diff/1/chrome/test/data/extensions... File chrome/test/data/extensions/tray_cast/manifest.json (right): https://codereview.chromium.org/1224643008/diff/1/chrome/test/data/extensions... chrome/test/data/extensions/tray_cast/manifest.json:1: { On 2015/07/08 22:23:55, achuithb wrote: > Add a comment like: > // chrome-extension://hlgmmjhlnlapooncikdpiiokdjcdpjme/ > > This way this file will show up in searches for this string Done.
https://codereview.chromium.org/1224643008/diff/20001/chrome/test/data/extens... File chrome/test/data/extensions/tray_cast/background.js (right): https://codereview.chromium.org/1224643008/diff/20001/chrome/test/data/extens... chrome/test/data/extensions/tray_cast/background.js:3: tryCreateActivity = function(id, title, tabId) { Add a function comment https://codereview.chromium.org/1224643008/diff/20001/chrome/test/data/extens... chrome/test/data/extensions/tray_cast/background.js:28: if (this !== backgroundSetup) What's this about? Add a comment https://codereview.chromium.org/1224643008/diff/20001/chrome/test/data/extens... chrome/test/data/extensions/tray_cast/background.js:36: wasStopMirroringCalledWithUserStop = function() { function comment https://codereview.chromium.org/1224643008/diff/20001/chrome/test/data/extens... chrome/test/data/extensions/tray_cast/background.js:47: if (item.activity != null) Is it necessary to add any checks here that at least one item.activity is non-null? https://codereview.chromium.org/1224643008/diff/20001/chrome/test/data/extens... chrome/test/data/extensions/tray_cast/background.js:66: if (item.receiver.id == receiverId) { Is it necessary to add any extra checks here to ensure that at least one receiver id matches?
https://codereview.chromium.org/1224643008/diff/20001/chrome/test/data/extens... File chrome/test/data/extensions/tray_cast/background.js (right): https://codereview.chromium.org/1224643008/diff/20001/chrome/test/data/extens... chrome/test/data/extensions/tray_cast/background.js:3: tryCreateActivity = function(id, title, tabId) { On 2015/07/09 21:40:10, achuithb wrote: > Add a function comment Done. https://codereview.chromium.org/1224643008/diff/20001/chrome/test/data/extens... chrome/test/data/extensions/tray_cast/background.js:28: if (this !== backgroundSetup) On 2015/07/09 21:40:10, achuithb wrote: > What's this about? Add a comment Done. https://codereview.chromium.org/1224643008/diff/20001/chrome/test/data/extens... chrome/test/data/extensions/tray_cast/background.js:36: wasStopMirroringCalledWithUserStop = function() { On 2015/07/09 21:40:10, achuithb wrote: > function comment It'd be something along the lines of "Returns true if stop mirroring was called with 'user-stop'", which is essentially a rephrasing of the method name. If you feel strongly, I can add a comment, but I'd prefer not to since the method name is already so verbose. https://codereview.chromium.org/1224643008/diff/20001/chrome/test/data/extens... chrome/test/data/extensions/tray_cast/background.js:47: if (item.activity != null) On 2015/07/09 21:40:10, achuithb wrote: > Is it necessary to add any checks here that at least one item.activity is > non-null? Sure, I can do that. It will help verify that stopMirroring is not called when there are no receivers. https://codereview.chromium.org/1224643008/diff/20001/chrome/test/data/extens... chrome/test/data/extensions/tray_cast/background.js:66: if (item.receiver.id == receiverId) { On 2015/07/09 21:40:10, achuithb wrote: > Is it necessary to add any extra checks here to ensure that at least one > receiver id matches? I'm not sure what value this will add. The testing code checks the calling value via `getLaunchDesktopMirroringReceiverId()`.
jdufault@chromium.org changed reviewers: + oshima@chromium.org
Oshima, would you mind taking a look? Thanks!
which part do you want me to review? I'm not the owner of any directories, nor familiar with the code.
lgtm
miu@chromium.org changed reviewers: + miu@chromium.org
I'm the owner of the tab capture API. What is this for and how is it used? It doesn't look like anything is actually run (just a bunch of JS function definitions, FWICT).
miu@chromium.org changed reviewers: - oshima@chromium.org
Removing oshima@...
On 2015/07/17 20:34:45, miu wrote: > Removing oshima@... miu@, see https://codereview.chromium.org/1231593002/ for a browser-test for the ChromeOS ash status tray cast integration - it uses this extension so that we don't actually startup/stop casts, plus we get extra verification. This doesn't have any changes w.r.t. the tap capture API except to add the extension to the whitelist.
https://codereview.chromium.org/1224643008/diff/40001/chrome/browser/extensio... File chrome/browser/extensions/api/tab_capture/tab_capture_api.cc (right): https://codereview.chromium.org/1224643008/diff/40001/chrome/browser/extensio... chrome/browser/extensions/api/tab_capture/tab_capture_api.cc:68: "hlgmmjhlnlapooncikdpiiokdjcdpjme", // Test cast extension Rather than add a string to production code that only applies to tests, can you just whitelist the extension ID in the test code? For example, here's how the TabCaptureApiTest does it: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext...
On 2015/07/17 23:23:14, miu wrote: > https://codereview.chromium.org/1224643008/diff/40001/chrome/browser/extensio... > File chrome/browser/extensions/api/tab_capture/tab_capture_api.cc (right): > > https://codereview.chromium.org/1224643008/diff/40001/chrome/browser/extensio... > chrome/browser/extensions/api/tab_capture/tab_capture_api.cc:68: > "hlgmmjhlnlapooncikdpiiokdjcdpjme", // Test cast extension > Rather than add a string to production code that only applies to tests, can you > just whitelist the extension ID in the test code? For example, here's how the > TabCaptureApiTest does it: > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext... I'm not familiar with what precisely that command line flag does, but loading it into Chrome isn't the issue; instead, we're using that list to identify which extensions support the API methods we need. See https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...
On 2015/07/17 23:27:55, jdufault wrote: > On 2015/07/17 23:23:14, miu wrote: > https://codereview.chromium.org/1224643008/diff/40001/chrome/browser/extensio... > > chrome/browser/extensions/api/tab_capture/tab_capture_api.cc:68: > > "hlgmmjhlnlapooncikdpiiokdjcdpjme", // Test cast extension > > Rather than add a string to production code that only applies to tests, can > you > > just whitelist the extension ID in the test code? > > I'm not familiar with what precisely that command line flag does, but loading it > into Chrome isn't the issue; instead, we're using that list to identify which > extensions support the API methods we need. OIC. Interesting problem. Okay, lgtm, but could you please document this in the comment (on line 60)? Something like: // ...existing comment here... // // This list is also used by CastConfigDelegateChromeos to find official Cast Extensions. BTW--You may want to consult one of the chrome/browser/extensions/OWNERS to see if there is a better mechanism (or precedent) for doing this look-up.
The CQ bit was checked by jdufault@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from achuith@chromium.org, miu@chromium.org Link to the patchset: https://codereview.chromium.org/1224643008/#ps60001 (title: "add comment before landing")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1224643008/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by jdufault@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from achuith@chromium.org, miu@chromium.org Link to the patchset: https://codereview.chromium.org/1224643008/#ps80001 (title: "Add license")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1224643008/80001
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/2eb5b206baa2a20876658ed1d8e453f6c8a21a80 Cr-Commit-Position: refs/heads/master@{#339900} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
