New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Preparing chrome driver to download in headless mode #5262

Closed

IlyaNaumovich wants to merge 4 commits into SeleniumHQ:trunk from IlyaNaumovich:downloadHeadlesChromePreparation

IlyaNaumovich commented Dec 26, 2017 •

edited by AutomatedTester

Loading

The method send commands to the browser to avoid blocking
of downloading in headless mode.
(The issue is described here:
#5159)

By placing an X in the preceding checkbox, I verify that I have signed the Contributor License Agreement

This change is


          Preparing chrome driver to download in headless mode

aa2afab

The method send commands to the browser to avoid blocking
of downloading in headless mode.
(The issue is described here:
SeleniumHQ#5159)

Member

p0deje commented Dec 28, 2017

Instead of providing a single method which sets this command, we should actually provide a method that allows sending arbitrary commands to DevTools. After that, anyone can easily use it to allow downloading.

In Ruby, such API can look like this:

driver.send_chromium_command(cmd: 'Page.setDownloadBehavior', params: {behavior: 'allow', downloadPath: '/User/paulo/projects/app/tmp'})

p0deje added the C-java label

Member

barancev commented Dec 28, 2017

Also don't forget adding tests that cover new code.

Member

barancev commented Dec 28, 2017

By the way, why do you create a new http client instead of using CommandExecutor to send a command?

Member

barancev commented Jan 8, 2018

I've thought about this feature a bit more. Yes, a function to send custom commands is a good thing, we should add it. But in this special case it looks like a workaround. Implementing a capability that enables/disables file download would be much more in line with Selenium strategy to handle such features.

Member

shs96c commented Jan 8, 2018

Why isn't this simply making a change to the ChromeDriverCommandExecutor? Maybe I've missed something....


          The method send commands to the browser to avoid blocking

6785a3f

of downloading in headless mode.
(The issue is described here:
SeleniumHQ#5159)

shs96c reviewed

View reviewed changes

Member

shs96c left a comment

This is super-close to being ready. Thanks for waiting for us.

I've added a couple of "stylistic comments": we can fix those ourselves, but it'd be nice if you could fix those too.

java/client/src/org/openqa/selenium/remote/http/JsonHttpCommandCodec.java Outdated

@@ @@ -133,6 +134,8 @@ public JsonHttpCommandCodec() { @@
                   defineCommand(TOUCH_MOVE, post("/session/:sessionId/touch/move"));
                   defineCommand(TOUCH_SCROLL, post("/session/:sessionId/touch/scroll"));
                   defineCommand(TOUCH_UP, post("/session/:sessionId/touch/up"));
+                  defineCommand(SEND_COMMAND_TO_BROWSER, post("/session/:sessionId/chromium/send_command_and_get_result"));

Member

shs96c Jan 15, 2018

This is Chrome specific, and so shouldn't be in the general JsonHttpCommandCodec. The best place is ChromeDriverCommandExecutor

java/client/src/org/openqa/selenium/chrome/ChromeDriver.java Outdated

                 @Deprecated
                 public ChromeDriver(ChromeDriverService service, Capabilities capabilities) {
                   super(new ChromeDriverCommandExecutor(service), capabilities);
+                  if (capabilities.getCapability("enableDownloading")!=null){

Member

shs96c Jan 15, 2018

Stylistic comment: Prefer a little more space around things: ) != null) {

java/client/src/org/openqa/selenium/chrome/ChromeDriver.java Outdated

                 @Deprecated
                 public ChromeDriver(ChromeDriverService service, Capabilities capabilities) {
                   super(new ChromeDriverCommandExecutor(service), capabilities);
+                  if (capabilities.getCapability("enableDownloading")!=null){
+                    sendCommandForDownloadChromeHeadLess(
+                        (String) capabilities.getCapability("enableDownloading"));

Member

shs96c Jan 15, 2018

You may get a ClassCastException here. There are two ways to avoid this: either make the check above be capabilities.getCapability("enableDownloading") instanceof String, or use String.valueOf here.

java/client/src/org/openqa/selenium/chrome/ChromeDriver.java Outdated

                 @Deprecated
                 public ChromeDriver(ChromeDriverService service, Capabilities capabilities) {
                   super(new ChromeDriverCommandExecutor(service), capabilities);
+                  if (capabilities.getCapability("enableDownloading")!=null){
+                    sendCommandForDownloadChromeHeadLess(
+                        (String) capabilities.getCapability("enableDownloading"));

Member

shs96c Jan 15, 2018

enableDownloading is not a legal capability name. chromium:enableDownloading is. The reasoning is here: https://w3c.github.io/webdriver/webdriver-spec.html#dfn-extension-capability

Contributor

jleyba Jan 17, 2018

nit: We're not the Chromium project and shouldn't introduce capabilities in their namespace.

How about using the selenium:chrome prefix?

java/client/src/org/openqa/selenium/chrome/ChromeDriver.java Outdated

+                 * @param downloadPath
+                 */
+                public void sendCommandForDownloadChromeHeadLess(String downloadPath) {
+                  if (downloadPath.length()>0){

Member

shs96c Jan 15, 2018

What happens if the path is zero length? That seems like an unusual position to be in. Log or throw an error?

java/client/src/org/openqa/selenium/chrome/ChromeDriver.java Outdated

+                 * @param downloadPath
+                 */
+                public void sendCommandForDownloadChromeHeadLess(String downloadPath) {
+                  if (downloadPath.length()>0){

Member

shs96c Jan 15, 2018

Stylistic comment: if (!downloadPath.isEmpty()) { is preferable.

java/client/src/org/openqa/selenium/chrome/ChromeDriverCommand.java Outdated

@@ @@ -24,4 +24,7 @@ @@
                 private ChromeDriverCommand() {}
                 static final String LAUNCH_APP = "launchApp";
+                static final String SEND_COMMANDS_FOR_DOWNLOAD_CHROME_HEAD_LESS
+                    = "sendCommandForDownloadChromeHeadLess";

Member

shs96c Jan 15, 2018

Nit: Headless is one word, not two, and so doesn't need camelcasing in this way.

java/client/src/org/openqa/selenium/chrome/ChromeOptions.java Outdated

+                  if (enableDownloading){
+                    setCapability(ENABLE_DOWNLOADING, downloadPath);
+                  }else {
+                    setCapability(ENABLE_DOWNLOADING, "");

Member

shs96c Jan 15, 2018

Setting the capability to null will remove it from the map, which is probably more desirable.

java/client/src/org/openqa/selenium/chrome/ChromeOptions.java Outdated

                   return this;
                 }
+                public ChromeOptions setEnableDownloading(boolean enableDownloading, String downloadPath){
+                  if (enableDownloading){

Member

shs96c Jan 15, 2018

Stylistic note: whitespace around braces: if (enableDownloading) {.


          The method send commands to the browser to avoid blocking

cd99e1a

jleyba reviewed

View reviewed changes

java/client/src/org/openqa/selenium/remote/CapabilityType.java Outdated

@@ @@ -47,6 +47,7 @@ @@
                 String ELEMENT_SCROLL_BEHAVIOR = "elementScrollBehavior";
                 String HAS_TOUCHSCREEN = "hasTouchScreen";
                 String OVERLAPPING_CHECK_DISABLED = "overlappingCheckDisabled";
+                String ENABLE_DOWNLOADING = "chromium:enableDownloading";

Contributor

jleyba Jan 17, 2018

The name enableDownloading implies this is a boolean capability. How about downloadDir?

Author

IlyaNaumovich Jan 19, 2018

Thank you for review. I believe that "downloadDir" is not so appropriate in this case because it sounds like specifying download directory rather than applying download behavior for specific path. It could cause some misunderstanding as to why we introduce this capability. What about AllowHeadlessDownloadTo?
From one hand, it is boolean, from the other hand, it specify the downloading directory. There are no ways to allow downloading without specifying directory for downloading. There are no way to specify directory for downloading without allowing downloading.

Contributor

jleyba Jan 19, 2018

It's not specific to headless, so how about allowDownloadsTo?


          The method send commands to the browser to avoid blocking

81858d5

of downloading in headless mode.
(The issue is described here:
SeleniumHQ#5159)

Author

IlyaNaumovich commented Feb 6, 2018

@barancev, @shs96c, @jleyba I'm just wondering, maybe you want me to add something to this pull request

Member

lmtierney commented Mar 9, 2018

After talking with the chromedriver team, the send_command will be moved to a w3c-compliant protocol extension endpoint (https://w3c.github.io/webdriver/webdriver-spec.html#protocol-extensions). We may want to hold off on this PR until the change.

See the issue raised in the chromium bugtracker: https://bugs.chromium.org/p/chromedriver/issues/detail?id=2307

Member

shs96c commented Jun 15, 2018

Do I need to continue looking at this PR?

shs96c added the R-awaiting answer label

timwindsor commented Jul 3, 2018 •

edited

Loading

The chromium change is to the api interface but they still support the call used in this PR. The changes here are necessary in order to use the API that chromium is providing.

Oh - perhaps this is taken care of by #5989 instead?

joywave mentioned this pull request

Java implementation of DevTools API #6667

Closed

CLAassistant commented Nov 23, 2019 •

edited

Loading

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

Member

AutomatedTester commented Mar 25, 2020

@shs96c Are we still going to accept this patch?

diemol closed this

diemol reopened this

diemol changed the base branch from master to trunk

July 12, 2020 20:05

Member

diemol commented Jun 4, 2021

This went stale a while ago, and probably it should be possible through the new interface with CDP. If there is the need to have this in the Java tree, please talk to us in our Slack/IRC channel.

diemol closed this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

C-java R-awaiting answer