Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2361453004:
Add browsertest for Flash permission prompt (Closed)
DescriptionAdd browsertest for Flash permission prompt
This adds a browsertest for the Flash permission prompt. The test has been
written in such a way that it can be reused for other permission prompts.
BUG=641615, 644052
Committed: https://crrev.com/c86bb867d2aacba38e61b68679586fa58b45368e
Cr-Commit-Position: refs/heads/master@{#422050}
Patch Set 1 #Patch Set 2 : Add browesrtest for Flash permission prompt #Patch Set 3 : Add browesrtest for Flash permission prompt #Patch Set 4 : Add browesrtest for Flash permission prompt #
Total comments: 19
Patch Set 5 : Add browesrtest for Flash permission prompt #Patch Set 6 : Test #Patch Set 7 : Test #
Total comments: 2
Patch Set 8 : Test #Patch Set 9 : Test #Patch Set 10 : Test #
Depends on Patchset: Messages
Total messages: 37 (23 generated)
Description was changed from ========== Add browesrtest for Flash permission prompt This adds a browsertest for the Flash permission prompt. The test has been written in such a way that it can be reused for other permission prompts. BUG=641615 ========== to ========== Add browesrtest for Flash permission prompt This adds a browsertest for the Flash permission prompt. The test has been written in such a way that it can be reused for other permission prompts. BUG=641615,644052 ==========
raymes@chromium.org changed reviewers: + dominickn@chromium.org, tommycli@chromium.org
just noticed below: https://codereview.chromium.org/2361453004/diff/60001/content/public/test/ppa... File content/public/test/ppapi_test_utils.h (right): https://codereview.chromium.org/2361453004/diff/60001/content/public/test/ppa... content/public/test/ppapi_test_utils.h:37: bool RegisterFlashTestPlugin(base::CommandLine* command_line) Oh, I really like this! This a smart fake Flash!
ping :) dominickn, tommycli could you ptal?
Looks pretty good - are you going to make this more general or leave it specialised for Flash right now? Nit: fix "browsertest" in CL name. https://codereview.chromium.org/2361453004/diff/60001/chrome/browser/permissi... File chrome/browser/permissions/permissions_browsertest.cc (right): https://codereview.chromium.org/2361453004/diff/60001/chrome/browser/permissi... chrome/browser/permissions/permissions_browsertest.cc:115: void SetUpInProcessBrowserTestFixture() override {} Nit: remove the do-nothing override? https://codereview.chromium.org/2361453004/diff/60001/chrome/browser/permissi... chrome/browser/permissions/permissions_browsertest.cc:134: LOG(INFO) << "Running test for: " << current_test_->test_name(); Nit: remove LOG https://codereview.chromium.org/2361453004/diff/60001/chrome/browser/permissi... chrome/browser/permissions/permissions_browsertest.cc:156: prompt_factory()->set_response_type(PermissionRequestManager::DISMISS); Nit: check that the current count is 0?
Also looks good. Same question as dom about the complexity. https://codereview.chromium.org/2361453004/diff/60001/chrome/browser/permissi... File chrome/browser/permissions/permissions_browsertest.cc (right): https://codereview.chromium.org/2361453004/diff/60001/chrome/browser/permissi... chrome/browser/permissions/permissions_browsertest.cc:29: class PermissionTestConfig { The abstraction seems good - and I don't know permissions code that well - but are we definitely going to reuse this interface? Because if not, maybe we should follow the You Ain't Gonna Need It and just have one concrete test for Flash. It could be a lot simpler Or - if we are going to reuse it, it might be nice to put the test config and PermissionsBrowserTest in a test support file. That can be in a separate patch if that makes it easier... The test just seems complex. https://codereview.chromium.org/2361453004/diff/60001/chrome/browser/permissi... chrome/browser/permissions/permissions_browsertest.cc:75: command_line->AppendSwitch(switches::kEnablePepperTesting); Would recommend inlining kEnablePepperTesting within RegisterFlashTestPlugin, since that plugin crashes without that flag https://codereview.chromium.org/2361453004/diff/60001/chrome/browser/permissi... chrome/browser/permissions/permissions_browsertest.cc:78: switches::kOverridePluginPowerSaverForTesting, "ignore-list"); ignore-list unneeded if we're registering the plugin as Flash anyways. https://codereview.chromium.org/2361453004/diff/60001/chrome/test/data/permis... File chrome/test/data/permissions/flash.html (right): https://codereview.chromium.org/2361453004/diff/60001/chrome/test/data/permis... chrome/test/data/permissions/flash.html:17: plugin.addEventListener('message', handleEvent); nit: I've seen this cool pattern recently: plugin.addEventListener('message', function handleEvent(e) { if (event.data.source === 'getPowerSaverStatusResponse') { plugin.removeEventListener('message', handleEvent); window.domAutomationController.send(true); } }); (The handleEvent is inline) https://codereview.chromium.org/2361453004/diff/60001/chrome/test/data/permis... chrome/test/data/permissions/flash.html:30: <a href="https://get.adobe.com/flashplayer/" id="flash-link">Download Flash.</a> need line wrap here? https://codereview.chromium.org/2361453004/diff/60001/content/public/test/ppa... File content/public/test/ppapi_test_utils.cc (right): https://codereview.chromium.org/2361453004/diff/60001/content/public/test/ppa... content/public/test/ppapi_test_utils.cc:103: PluginInfo(library_name, FILE_PATH_LITERAL("#Shockwave Flash#"), I noticed that if I didn't set the version to 100.0, it would trigger the Outdated plugin path and tests would fail.
Description was changed from ========== Add browesrtest for Flash permission prompt This adds a browsertest for the Flash permission prompt. The test has been written in such a way that it can be reused for other permission prompts. BUG=641615,644052 ========== to ========== Add browsertest for Flash permission prompt This adds a browsertest for the Flash permission prompt. The test has been written in such a way that it can be reused for other permission prompts. BUG=641615,644052 ==========
> Looks pretty good - are you going to make this more general or leave it specialised for Flash right now? I've paramaterized it now so it's ready to go for new permissions too. https://codereview.chromium.org/2361453004/diff/60001/chrome/browser/permissi... File chrome/browser/permissions/permissions_browsertest.cc (right): https://codereview.chromium.org/2361453004/diff/60001/chrome/browser/permissi... chrome/browser/permissions/permissions_browsertest.cc:29: class PermissionTestConfig { On 2016/09/26 22:54:49, tommycli wrote: > The abstraction seems good - and I don't know permissions code that well - but > are we definitely going to reuse this interface? > > Because if not, maybe we should follow the You Ain't Gonna Need It and just have > one concrete test for Flash. It could be a lot simpler > > Or - if we are going to reuse it, it might be nice to put the test config and > PermissionsBrowserTest in a test support file. That can be in a separate patch > if that makes it easier... > > The test just seems complex. Yes we'll definitely reuse it - I have an outstanding bug to add a framework like this: see https://bugs.chromium.org/p/chromium/issues/detail?id=644052 I can move FlashPermissionTestConfig into a separate file if you like? https://codereview.chromium.org/2361453004/diff/60001/chrome/browser/permissi... chrome/browser/permissions/permissions_browsertest.cc:75: command_line->AppendSwitch(switches::kEnablePepperTesting); On 2016/09/26 22:54:49, tommycli wrote: > Would recommend inlining kEnablePepperTesting within RegisterFlashTestPlugin, > since that plugin crashes without that flag Done. https://codereview.chromium.org/2361453004/diff/60001/chrome/browser/permissi... chrome/browser/permissions/permissions_browsertest.cc:78: switches::kOverridePluginPowerSaverForTesting, "ignore-list"); On 2016/09/26 22:54:49, tommycli wrote: > ignore-list unneeded if we're registering the plugin as Flash anyways. Done. https://codereview.chromium.org/2361453004/diff/60001/chrome/browser/permissi... chrome/browser/permissions/permissions_browsertest.cc:115: void SetUpInProcessBrowserTestFixture() override {} On 2016/09/26 07:28:59, dominickn wrote: > Nit: remove the do-nothing override? Done. https://codereview.chromium.org/2361453004/diff/60001/chrome/browser/permissi... chrome/browser/permissions/permissions_browsertest.cc:134: LOG(INFO) << "Running test for: " << current_test_->test_name(); On 2016/09/26 07:28:59, dominickn wrote: > Nit: remove LOG The log will be important when there is a paramaterized test because all you will see in the test output is a number denoting which test is running. This will give more context. https://codereview.chromium.org/2361453004/diff/60001/chrome/browser/permissi... chrome/browser/permissions/permissions_browsertest.cc:156: prompt_factory()->set_response_type(PermissionRequestManager::DISMISS); On 2016/09/26 07:28:59, dominickn wrote: > Nit: check that the current count is 0? Done. https://codereview.chromium.org/2361453004/diff/60001/chrome/test/data/permis... File chrome/test/data/permissions/flash.html (right): https://codereview.chromium.org/2361453004/diff/60001/chrome/test/data/permis... chrome/test/data/permissions/flash.html:17: plugin.addEventListener('message', handleEvent); Cool, thanks! https://codereview.chromium.org/2361453004/diff/60001/chrome/test/data/permis... chrome/test/data/permissions/flash.html:30: <a href="https://get.adobe.com/flashplayer/" id="flash-link">Download Flash.</a> On 2016/09/26 22:54:49, tommycli wrote: > need line wrap here? Done. https://codereview.chromium.org/2361453004/diff/60001/content/public/test/ppa... File content/public/test/ppapi_test_utils.cc (right): https://codereview.chromium.org/2361453004/diff/60001/content/public/test/ppa... content/public/test/ppapi_test_utils.cc:103: PluginInfo(library_name, FILE_PATH_LITERAL("#Shockwave Flash#"), On 2016/09/26 22:54:49, tommycli wrote: > I noticed that if I didn't set the version to 100.0, it would trigger the > Outdated plugin path and tests would fail. Done.
The CQ bit was checked by raymes@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: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
LGTM. Thanks for the end-to-end HBD test! (It does cover every aspect right?) I am a little scared of the test complexity right now, but if you're going to re-use that infrastructure for other permission types, then it makes a lot of sense.
On 2016/09/27 16:14:40, tommycli wrote: > LGTM. Thanks for the end-to-end HBD test! (It does cover every aspect right?) It tests loading the page, that flash is disabled, triggering a prompt and then that it gets enabled/disabled. There are several other end-to-end cases we could add tests for, for example flows that don't involve the prompt (if flash was already allowed). > I am a little scared of the test complexity right now, but if you're going to > re-use that infrastructure for other permission types, then it makes a lot of > sense.
Please take another look. When I tried to add a more custom flash test it seemed complicated, so along with Tommy's feedback I've taken a simpler approach that requires slightly more copy/pasting.
The CQ bit was checked by raymes@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: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
lgtm https://codereview.chromium.org/2361453004/diff/120001/chrome/browser/permiss... File chrome/browser/permissions/permissions_browsertest.h (right): https://codereview.chromium.org/2361453004/diff/120001/chrome/browser/permiss... chrome/browser/permissions/permissions_browsertest.h:21: // tests functions that are provided (see the functions with the Common* Nit: "test functions"
The CQ bit was checked by raymes@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...
https://codereview.chromium.org/2361453004/diff/120001/chrome/browser/permiss... File chrome/browser/permissions/permissions_browsertest.h (right): https://codereview.chromium.org/2361453004/diff/120001/chrome/browser/permiss... chrome/browser/permissions/permissions_browsertest.h:21: // tests functions that are provided (see the functions with the Common* On 2016/09/29 03:08:48, dominickn wrote: > Nit: "test functions" Done.
The CQ bit was checked by raymes@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 checked by raymes@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: This issue passed the CQ dry run.
The CQ bit was checked by raymes@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tommycli@chromium.org, dominickn@chromium.org Link to the patchset: https://codereview.chromium.org/2361453004/#ps180001 (title: "Test")
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.
Description was changed from ========== Add browsertest for Flash permission prompt This adds a browsertest for the Flash permission prompt. The test has been written in such a way that it can be reused for other permission prompts. BUG=641615,644052 ========== to ========== Add browsertest for Flash permission prompt This adds a browsertest for the Flash permission prompt. The test has been written in such a way that it can be reused for other permission prompts. BUG=641615,644052 ==========
Message was sent while issue was closed.
Committed patchset #10 (id:180001)
Message was sent while issue was closed.
Description was changed from ========== Add browsertest for Flash permission prompt This adds a browsertest for the Flash permission prompt. The test has been written in such a way that it can be reused for other permission prompts. BUG=641615,644052 ========== to ========== Add browsertest for Flash permission prompt This adds a browsertest for the Flash permission prompt. The test has been written in such a way that it can be reused for other permission prompts. BUG=641615,644052 Committed: https://crrev.com/c86bb867d2aacba38e61b68679586fa58b45368e Cr-Commit-Position: refs/heads/master@{#422050} ==========
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/c86bb867d2aacba38e61b68679586fa58b45368e Cr-Commit-Position: refs/heads/master@{#422050} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
