Add Print page support in Java #8991

raju249 · 2020-12-22T08:49:17Z

Thanks for contributing to Selenium!
A PR well described will help maintainers to quickly review and merge it

Before submitting your PR, please check our contributing guidelines.
Avoid large PRs, help reviewers by making them as simple and short as possible.

Description

Motivation and Context

Fixes #8802

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Checklist

I have read the contributing document.
My change requires a change to the documentation.
I have updated the documentation accordingly.
I have added tests to cover my changes.
All new and existing tests passed.

luke-hill · 2021-01-05T10:30:00Z

Just as an FYI (Code aside). If you notice any areas where the API in java is vastly different or needs a lot more work than Ruby, it's probably work flagging it at first instance, as I know it would be good to get these looking reasonably similar.

shs96c · 2021-01-05T10:16:29Z

java/client/src/org/openqa/selenium/printoptions/PageMargin.java

+// specific language governing permissions and limitations
+// under the License.
+
+package org.openqa.selenium.printoptions;


s/printoptions/print/

shs96c · 2021-01-05T10:17:05Z

java/client/src/org/openqa/selenium/printoptions/PageMargin.java

+  }
+
+  public void setTop(double top) {
+    if (top < 0) {


Use Require.positive

shs96c · 2021-01-05T10:17:43Z

java/client/src/org/openqa/selenium/printoptions/PageMargin.java

+  }
+
+  public double getTop() {
+    return this.top;


We don't use this unless it's strictly necessary or (often) in a constructor. Our IDE will let us know that this is a field.

shs96c · 2021-01-05T10:18:24Z

java/client/src/org/openqa/selenium/printoptions/PageMargin.java

+    this.left = left;
+  }
+
+  public Map<String, Double> toJson() {


No need for this method. The default serialisation mechanism will create an object of this shape by default.

In addition, please don't expose the json methods as public. They're not part of the user-facing API of Selenium

shs96c · 2021-01-05T10:19:28Z

java/client/src/org/openqa/selenium/printoptions/PageSize.java

+    this.width = width;
+  }
+
+  public Map<String, Double> toJson() {


This should not public and given how our json serialiser works, is not necessary.

shs96c · 2021-01-05T10:38:20Z

java/client/src/org/openqa/selenium/remote/RemoteWebDriver.java

@@ -331,6 +332,11 @@ public WebElement findElement(By locator) {
    }
  }

+  public String printPage(PrintOptions options) {


If this is something we expect all WebDriver instances to support (and we do, since it's in the spec) this could be "somewhere" on WebDriver and not only on RemoteWebDriver. However, we try and keep the API of the main interfaces as compact as possible, and use role-based interfaces where necessary (which is why there are so many of them --- even for JS execution)

Printing a page feels very similar to taking a screenshot, so we have two options to make this "feel" like a webdriver API. The first is to add a new method to TakesScreenshot (a default one that throws an exception, to keep our promise of Se4 being a drop-in replacement) The second is to add a new role-based interfaces (eg. PrintsPage) I guess a final option is to add a print method to WebDriver.Window, but that doesn't feel quite right.

Finally, the return type of this method tells us nothing. It's fine to use a tiny type for this kind of thing. In TakesScreenshot we use an OutputType. If we could munge things to and from PDF without additional libraries, I'd suggest adding PDF to OutputType and changing the print signature to be print(OutputType<X> as), but that's not an option I'm entirely comfortable with as PDFBox adds about 4MB to the size of Selenium. As such, I'd recommend declaring a Pdf class and returning that.

shs96c · 2021-01-05T10:39:39Z

java/client/test/org/openqa/selenium/chrome/ChromePrintCommandTest.java

+import org.openqa.selenium.printoptions.PrintOptions;
+import org.openqa.selenium.testing.JUnit4TestBase;
+
+public class ChromePrintCommandTest extends JUnit4TestBase {


Why is this chrome only? We expect Edge, Firefox and Safari to all support this functionality.

shs96c · 2021-01-05T10:40:54Z

java/client/test/org/openqa/selenium/chrome/ChromePrintCommandTest.java

+  private static String MAGIC_STRING = "JVBER";
+
+  @Before
+  public void setUp() {


Once you've added a role-based interface, the thing to do here would be to Assume that the driver implements the interface. If it does, cast to it and store on a field.

I'd also suggest navigating to a page: some browsers are in weird states when they start

shs96c · 2021-01-05T10:41:25Z

java/client/test/org/openqa/selenium/chrome/ChromePrintCommandTest.java

+    driver = new ChromeDriver(options);
+    driver.get(pages.printPage);
+  }
+  @After


Nit: empty line before @After to give the methods some room to breathe. If you follow the advice above, this isn't needed.

shs96c · 2021-01-05T10:42:23Z

java/client/test/org/openqa/selenium/firefox/FireFoxPrintCommandTest.java

+import org.openqa.selenium.printoptions.PrintOptions;
+import org.openqa.selenium.testing.JUnit4TestBase;
+
+public class FireFoxPrintCommandTest extends JUnit4TestBase {


o_O Why is this not the same as the first test?

Chrome needs to be headless, if not it throws an exception. Firefox doesn't need to headless.

This still needs to be lifted up to the main test suite, and this class removed. If chrome can't handle this when not headless we should file a bug with the chrome team, ignore the test for chrome, and link to that bug in the ignore's comment.

raju249 · 2021-01-05T12:21:45Z

Just as an FYI (Code aside). If you notice any areas where the API in java is vastly different or needs a lot more work than Ruby, it's probably work flagging it at first instance, as I know it would be good to get these looking reasonably similar.

Hey @luke-hill 👋
Yes, I understand your point. @AutomatedTester and I had a discussion about it while I was implementing for Ruby. I did the same before for ruby, but @barancev had a different opinion on that code, so I had to change it to as per their suggestions.

Regarding Java, I am following @AutomatedTester 's suggestions here, where I am keeping the implementation skeleton similar to what is done in .NET

Hope this helps!

shs96c

Looking good. Most of these are relatively small changes, I think.

shs96c · 2021-01-12T08:34:30Z

java/client/src/org/openqa/selenium/Pdf.java

+
+public class Pdf {
+
+  private Object base64String;


This should be final and a String as well. Since the type is now known, we could call this base64EncodedPdf or similar.

shs96c · 2021-01-12T08:34:45Z

java/client/src/org/openqa/selenium/Pdf.java

+    this.base64String = base64String;
+  }
+
+  public String getBase64String() {


getContent?

shs96c · 2021-01-12T08:34:56Z

java/client/src/org/openqa/selenium/Pdf.java

+  }
+
+  public String getBase64String() {
+    return (String) base64String;


This cast should not be required.

shs96c · 2021-01-12T08:37:25Z

java/client/src/org/openqa/selenium/PrintsPage.java

+package org.openqa.selenium;
+
+public interface PrintsPage {
+  <X> X print(Object printOptions) throws WebDriverException;


Pdf print(PrintOptions options)

Just returning X doesn't make it easy to use this API, since users don't know what X may be: once type erasure is applied, this ends up as Object. I'm not sure why you've created a PrintOptions class only to not use it here --- this seems like the ideal place, no?

shs96c · 2021-01-12T08:39:50Z

java/client/src/org/openqa/selenium/internal/Require.java

@@ -159,6 +159,20 @@ public static int positive(String argName, Integer number, String message) {
    return number;
  }

+  public static double positive(String argName, Double number, String message) {


Since you're returning a double, you should make number a primitive type and not a wrapper type. This prevents an unnecessary boxing from occurring and obviates the need for the null check.

shs96c · 2021-01-12T08:49:42Z

java/client/src/org/openqa/selenium/printoptions/PrintOptions.java

+    this.pageSize = pageSize;
+  }
+
+  public void setPageMargin(PageMargin margin) {


Check for null

shs96c · 2021-01-12T08:50:17Z

java/client/src/org/openqa/selenium/printoptions/PrintOptions.java

+    return this.pageSize;
+  }
+
+  public PageMargin getPageMargin() {


Try and keep getters and setters next to each other. I like to have the getter immediately above the setter, but as long as you're consistent it'll be fine.

shs96c · 2021-01-12T08:52:32Z

java/client/src/org/openqa/selenium/remote/RemoteWebDriver.java

@@ -322,6 +325,14 @@ public String getCurrentUrl() {
    }
  }

+  @Override
+  public Pdf print(Object printOptions) throws WebDriverException {


You do a cast immediately below to PrintOptions, and so you've made a (fast, easy to fix) compile-time error into a (found later in the development cycle, harder to identify and therefore fix) runtime error.

Pdf print(PrintOptions) please.

shs96c · 2021-01-12T08:53:48Z

java/client/src/org/openqa/selenium/remote/RemoteWebDriver.java

+  public Pdf print(Object printOptions) throws WebDriverException {
+    Response response = execute(DriverCommand.PRINT_PAGE((PrintOptions) printOptions));
+
+    Object result = response.getValue();


The rest of the methods do a cast and assume the type is right. You can do return new Pdf((String) result);

shs96c · 2021-01-12T08:54:33Z

java/client/test/org/openqa/selenium/firefox/FireFoxPrintCommandTest.java

+import org.openqa.selenium.printoptions.PrintOptions;
+import org.openqa.selenium.testing.JUnit4TestBase;
+
+public class FireFoxPrintCommandTest extends JUnit4TestBase {


This still needs to be lifted up to the main test suite, and this class removed. If chrome can't handle this when not headless we should file a bug with the chrome team, ignore the test for chrome, and link to that bug in the ignore's comment.

sonarqubecloud · 2021-01-18T09:54:24Z

SonarCloud Quality Gate failed.

0 Bugs
0 Vulnerabilities
0 Security Hotspots
5 Code Smells

0.0% Coverage
0.0% Duplication

shs96c

I think we're nearly there :) Thank you for the revisions!

shs96c · 2021-01-18T15:23:39Z

java/client/src/org/openqa/selenium/print/BUILD.bazel

+
+java_library(
+    name = "print",
+    srcs = glob(["**/*.java"]),


*.java. Globbing to an indeterminate depth is seldom what we want to do.

This file isn't needed at all

shs96c · 2021-01-18T15:24:58Z

java/client/src/org/openqa/selenium/print/BUILD.bazel

+    name = "print",
+    srcs = glob(["**/*.java"]),
+    visibility = [
+        "//visibility:public",


This should only be visible to the java_export target that this is part of (and, if necessary, other libraries that are part of that target). Public visibility is almost always a mistake for something that isn't meant to ship as a standalone component.

shs96c · 2021-01-18T15:30:28Z

java/client/src/org/openqa/selenium/internal/Require.java

+  public static double positive(String argName, double number, String message) {
+    if (number <= 0) {
+      if (message == null) {
+        throw new IllegalArgumentException(argName + " must be greater than 0");


Prefer adding a second positive(String, double) that delegates down to this three-param version. Using null in code is generally Not A Great Idea, and it looks ugly.

shs96c · 2021-01-18T15:32:26Z

java/client/src/org/openqa/selenium/print/BUILD.bazel

+        "//visibility:public",
+    ],
+    deps = [
+        "//java/client/src/org/openqa/selenium:core",


I'd suggest that this package needs to be exported by core, so you may just want to add print/*.java to core's definition.

java/client/src/org/openqa/selenium/print/PageMargin.java

java/client/test/org/openqa/selenium/PrintPageTest.java

shs96c · 2021-01-18T15:40:02Z

java/client/test/org/openqa/selenium/PrintPageTest.java

+  @Test
+  public void canPrintwoPages() {
+    PrintOptions printOptions = new PrintOptions();
+    printOptions.setPageRanges(new String[]{"1-2"});


If this is how we're expecting people to use this, then consider making setPageRanges take a mandatory first argument and String varargs to allow something like setPageRanges("1-2", "5-8")

java/client/test/org/openqa/selenium/print/PageSizeTest.java

java/client/test/org/openqa/selenium/print/PageMarginTest.java

shs96c · 2021-01-18T15:41:13Z

java/server/src/org/openqa/selenium/grid/data/NodeStatus.java

@@ -44,7 +44,7 @@
  public NodeStatus(
      NodeId nodeId,
      URI externalUri,
-      int maxSessionCount,
+      Integer maxSessionCount,


This change shouldn't be necessary for this PR. Please remove it.

Bazel build fails otherwise. I have removed it from this PR.

java/client/src/org/openqa/selenium/remote/BUILD.bazel

java/client/test/org/openqa/selenium/print/BUILD.bazel

shs96c

A couple of nits we can address in a follow up, but this looks fine.

shs96c · 2021-01-19T10:12:48Z

java/client/src/org/openqa/selenium/BUILD.bazel

@@ -25,6 +25,8 @@ java_export(
        "mobile/*.java",
        "net/*.java",
        "virtualauthenticator/*.java",
+        "print/*.java",
+


We don't need this empty line.

shs96c · 2021-01-19T10:15:14Z

java/client/src/org/openqa/selenium/print/PageMargin.java

+    return top;
+  }
+
+  public void setTop(double top) {


After discussion in the selenium-tlc channel, we're leaning towards immutable data-types. We can remove the setters in a follow-up PR, since this review has already gone on a while and that's a new change

raju249 added the C-java Java Bindings label Dec 22, 2020

raju249 self-assigned this Dec 22, 2020

raju249 changed the title ~~Add PrintOptions class and supporting classes for page margin and size~~ Add Print page support in Java Dec 23, 2020

raju249 force-pushed the print-page-java branch from 4e96560 to e977ed1 Compare December 28, 2020 06:09

raju249 marked this pull request as ready for review January 5, 2021 05:45

shs96c requested changes Jan 5, 2021

View reviewed changes

raju249 force-pushed the print-page-java branch from 4cdd12a to e8dbb31 Compare January 12, 2021 08:25

shs96c requested changes Jan 12, 2021

View reviewed changes

diemol mentioned this pull request Jan 14, 2021

Support for print-to-PDF #8802

Closed

raju249 added 19 commits January 18, 2021 10:44

Add PrintOptions class and supporting classes for page margin and size

44955d5

Add license string in margin and page classes

0c05ee0

Call execute to print page

cc3e987

Fix bazel build file

29ca25e

Add bazel build file to printoptions

ee8e898

Remove deps from printoptions build bazel file

b6595aa

Add Test class and return json for print options

f330eef

Add assertion to test with magic string

79d8735

Add test for multiple print options

0e052bd

Rename file with chrome specific tests

2fa568a

Add PrintPage tests for firefox

1926c59

Add PageSize tests

ca15415

Add PageMarginTests

163ebab

Add PrintOptions class tests

4ccccf4

Fix code smells

2dfcf10

Add a print package inside print class

369b07d

Fix chrome and firefox orientation test for print command

6735f3c

Add new line in @after

14bca93

Remove chrome test

38f8368

raju249 added 8 commits January 18, 2021 10:47

Add pdf class and PrintsPage interface

d6980c0

Fix unwanted changes

191af19

Change method signature

41e0c35

Add overloaded version of Require.positive for double

4a7dde4

Fix with suggestions for Print classess

fc8578e

Rename printoptions package to print

c80bddf

Require nonNull is Print methods

ce4b38d

Add PrintPage test in root level

aa897c9

raju249 force-pushed the print-page-java branch from 8d35e5f to aa897c9 Compare January 18, 2021 06:00

raju249 added 2 commits January 18, 2021 14:57

Change temporarily to Object class and remove PrintPage interface

eb4fb3f

Change integer type in NodeStatus

d37abc6

Revert to independent Interface class

d3a0356

shs96c requested changes Jan 18, 2021

View reviewed changes

AutomatedTester reviewed Jan 18, 2021

View reviewed changes

java/client/src/org/openqa/selenium/remote/BUILD.bazel Outdated Show resolved Hide resolved

java/client/test/org/openqa/selenium/print/BUILD.bazel Outdated Show resolved Hide resolved

raju249 added 2 commits January 19, 2021 11:10

Fix import error and remove print BUILD.bazel files

dc8a584

Add a comment for page size and change pageRanges setter

ce1ea4a

shs96c approved these changes Jan 19, 2021

View reviewed changes

shs96c merged commit 626f908 into SeleniumHQ:trunk Jan 19, 2021

raju249 deleted the print-page-java branch January 19, 2021 10:30

raju249 mentioned this pull request Jan 19, 2021

Add follow up changes from Print Page Java PR #9061

Merged

8 tasks

Add Print page support in Java #8991

Add Print page support in Java #8991

Conversation

raju249 commented Dec 22, 2020 • edited Loading

Description

Motivation and Context

Types of changes

Checklist

luke-hill commented Jan 5, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raju249 commented Jan 5, 2021 • edited Loading

shs96c left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarqubecloud bot commented Jan 18, 2021

shs96c left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shs96c left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raju249 commented Dec 22, 2020 •

edited

Loading

raju249 commented Jan 5, 2021 •

edited

Loading