Skip to content
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

fix: removed code smells and refactored project structure #141

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

fix: removed code smells and refactored project structure #141

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 16 additions & 12 deletions src/main/java/org/openqa/selenium/htmlunit/HtmlUnitAlert.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ public class HtmlUnitAlert implements Alert {

private final HtmlUnitDriver driver_;
private AlertHolder holder_;
private boolean quitting_;
private boolean autoDismissOnQuit;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

better naming makes sense but please stay with the overall rule to add a _ at the end

private final Lock lock_ = new ReentrantLock();
private final Condition condition_ = lock_.newCondition();
private WebWindow webWindow_;
Expand All @@ -60,7 +60,7 @@ public class HtmlUnitAlert implements Alert {
}

private void alertHandler(final Page page, final String message) {
if (quitting_) {
if (autoDismissOnQuit) {
return;
}
webWindow_ = page.getEnclosingWindow();
Expand All @@ -69,7 +69,7 @@ private void alertHandler(final Page page, final String message) {
}

private boolean confirmHandler(final Page page, final String message) {
if (quitting_) {
if (autoDismissOnQuit) {
return false;
}
webWindow_ = page.getEnclosingWindow();
Expand All @@ -83,21 +83,25 @@ private void awaitCondition() {
lock_.lock();
try {
if (driver_.isProcessAlert()) {
try {
condition_.await(5, TimeUnit.SECONDS);
}
catch (final InterruptedException e) {
throw new RuntimeException(e);
}
waitFor(5);
}
}
finally {
lock_.unlock();
}
}

private void waitFor(int time) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

any plans to reuse this somewhere else?

try {
condition_.await(time, TimeUnit.SECONDS);
}
catch (final InterruptedException e) {
throw new RuntimeException(e);
}
}

private String promptHandler(final Page page, final String message, final String defaultMessage) {
if (quitting_) {
if (autoDismissOnQuit) {
return null;
}
webWindow_ = page.getEnclosingWindow();
Expand All @@ -108,7 +112,7 @@ private String promptHandler(final Page page, final String message, final String
}

private boolean onbeforeunloadHandler(final Page page, final String returnValue) {
if (quitting_) {
if (autoDismissOnQuit) {
return true;
}
webWindow_ = page.getEnclosingWindow();
Expand All @@ -123,7 +127,7 @@ WebWindow getWebWindow() {
}

public void setAutoAccept(final boolean autoAccept) {
this.quitting_ = autoAccept;
this.autoDismissOnQuit = autoAccept;
}

public void handleBrowserCapabilities(final Capabilities capabilities) {
Expand Down
24 changes: 14 additions & 10 deletions src/main/java/org/openqa/selenium/htmlunit/HtmlUnitElementFinder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,7 +124,7 @@ public List<WebElement> findElements(final HtmlUnitDriver driver, final By locat
}

final List<DomElement> allElements = ((HtmlPage) lastPage).getElementsById(getValue(locator));
return convertRawDomElementsToWebElements(driver, allElements);
return HtmlUnitElementUtils.convertRawDomElementsToWebElements(driver, allElements);
}

@Override
Expand All @@ -150,7 +150,7 @@ public List<WebElement> findElements(final HtmlUnitDriver driver, final By locat
}

final List<DomElement> allElements = ((HtmlPage) lastPage).getElementsByName(getValue(locator));
return convertRawDomElementsToWebElements(driver, allElements);
return HtmlUnitElementUtils.convertRawDomElementsToWebElements(driver, allElements);
}

@Override
Expand Down Expand Up @@ -489,7 +489,7 @@ public WebElement findElement(final HtmlUnitWebElement element, final By locator
// selector is therefore
// invalid
throw new InvalidSelectorException(
String.format(INVALIDSELECTIONERROR, value, node.getClass().toString()));
String.format(INVALIDSELECTIONERROR, value, node.getClass()));
}

@Override
Expand All @@ -515,7 +515,7 @@ public List<WebElement> findElements(final HtmlUnitWebElement element, final By
// selector is
// therefore invalid
throw new InvalidSelectorException(
String.format(INVALIDSELECTIONERROR, value, e.getClass().toString()));
String.format(INVALIDSELECTIONERROR, value, e.getClass()));
}
}
return toReturn;
Expand Down Expand Up @@ -565,14 +565,18 @@ protected static String getValue(final By locator) {
}
}

private static List<WebElement> convertRawDomElementsToWebElements(
final HtmlUnitDriver driver, final List<DomElement> nodes) {
final List<WebElement> toReturn = new ArrayList<>(nodes.size());
public static class HtmlUnitElementUtils{

for (final DomElement node : nodes) {
toReturn.add(driver.toWebElement(node));
public static List<WebElement> convertRawDomElementsToWebElements(
final HtmlUnitDriver driver, final List<DomElement> nodes) {
final List<WebElement> toReturn = new ArrayList<>(nodes.size());

for (final DomElement node : nodes) {
toReturn.add(driver.toWebElement(node));
}

return toReturn;
}

return toReturn;
}
}
15 changes: 11 additions & 4 deletions src/main/java/org/openqa/selenium/htmlunit/HtmlUnitTargetLocator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -187,12 +187,14 @@ public WebElement activeElement() {
@Override
public Alert alert() {
final HtmlUnitAlert alert = driver_.getAlert();
final int SLEEP_TIME = 50;
final int NUMBER_OF_TRIES = 5;

if (!alert.isLocked()) {
for (int i = 0; i < 5; i++) {
for (int i = 0; i < NUMBER_OF_TRIES; i++) {
if (!alert.isLocked()) {
try {
Thread.sleep(50);
Thread.sleep(SLEEP_TIME);
}
catch (final InterruptedException e) {
throw new RuntimeException(e);
Expand All @@ -208,13 +210,18 @@ public Alert alert() {
final WebWindow alertWindow = alert.getWebWindow();
final WebWindow currentWindow = driver_.getCurrentWindow().getWebWindow();

if (alertWindow != currentWindow && !isChild(currentWindow, alertWindow)
&& !isChild(alertWindow, currentWindow)) {
if (isAlertTimedOut(alertWindow , currentWindow)) {
throw new TimeoutException();
}

return alert;
}

private static boolean isAlertTimedOut(WebWindow alertWindow , WebWindow currentWindow) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks a bit strange, when checking the method for figuring out why the alerts timed out i will not see anything regarding a timeout - maybe a better name?

return (alertWindow != currentWindow && !isChild(currentWindow, alertWindow)
&& !isChild(alertWindow, currentWindow));
}

private static boolean isChild(final WebWindow parent, final WebWindow potentialChild) {
for (WebWindow child = potentialChild; child != null; child = child.getParentWindow()) {
if (child == parent) {
Expand Down
54 changes: 32 additions & 22 deletions src/main/java/org/openqa/selenium/htmlunit/HtmlUnitWebElement.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,13 +81,6 @@
*/
public class HtmlUnitWebElement implements WrapsDriver, WebElement, Coordinates, Locatable {

private static final String[] booleanAttributes = {"async", "autofocus", "autoplay", "checked", "compact",
"complete", "controls", "declare", "defaultchecked", "defaultselected", "defer", "disabled", "draggable",
"ended", "formnovalidate", "hidden", "indeterminate", "iscontenteditable", "ismap", "itemscope", "loop",
"multiple", "muted", "nohref", "noresize", "noshade", "novalidate", "nowrap", "open", "paused", "pubdate",
"readonly", "required", "reversed", "scoped", "seamless", "seeking", "selected", "spellcheck", "truespeed",
"willvalidate"};

private final HtmlUnitDriver driver_;
private final int id_;
private final DomElement element_;
Expand Down Expand Up @@ -255,7 +248,7 @@ void switchFocusToThisIfNeeded() {
final boolean jsEnabled = driver_.isJavascriptEnabled();
final boolean oldActiveEqualsCurrent = oldActiveElement.equals(this);
try {
final boolean isBody = oldActiveElement.getTagName().toLowerCase().equals("body");
final boolean isBody = oldActiveElement.getTagName().equalsIgnoreCase("body");
if (jsEnabled && !oldActiveEqualsCurrent && !isBody) {
oldActiveElement.element_.blur();
}
Expand Down Expand Up @@ -285,6 +278,12 @@ public String getAttribute(final String name) {
assertElementNotStale();

final String lowerName = name.toLowerCase();
final String[] booleanAttributes = {"async", "autofocus", "autoplay", "checked", "compact",
"complete", "controls", "declare", "defaultchecked", "defaultselected", "defer", "disabled", "draggable",
"ended", "formnovalidate", "hidden", "indeterminate", "iscontenteditable", "ismap", "itemscope", "loop",
"multiple", "muted", "nohref", "noresize", "noshade", "novalidate", "nowrap", "open", "paused", "pubdate",
"readonly", "required", "reversed", "scoped", "seamless", "seeking", "selected", "spellcheck", "truespeed",
"willvalidate"};

if (element_ instanceof HtmlInput && ("selected".equals(lowerName) || "checked".equals(lowerName))) {
return trueOrNull(((HtmlInput) element_).isChecked());
Expand Down Expand Up @@ -410,21 +409,11 @@ public String getDomProperty(final String name) {
return ScriptRuntime.toCharSequence(ScriptableObject.getProperty(scriptable, lowerName)).toString();
}

// js disabled, fallback to some hacks
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a can't see any improvement from these changes in terms of readability

if ("disabled".equals(lowerName)) {
if (element_ instanceof DisabledElement) {
return trueOrFalse(((DisabledElement) element_).isDisabled());
}
}
String element_1 = disabledJsCheck(lowerName);
if (element_1 != null) return element_1;

if ("checked".equals(lowerName)) {
if (element_ instanceof HtmlCheckBoxInput) {
return trueOrFalse(((HtmlCheckBoxInput) element_).isChecked());
}
else if (element_ instanceof HtmlRadioButtonInput) {
return trueOrFalse(((HtmlRadioButtonInput) element_).isChecked());
}
}
String element_2 = isElementChecked(lowerName);
if (element_2 != null) return element_2;

final String value = element_.getAttribute(lowerName);
if (ATTRIBUTE_NOT_DEFINED == value) {
Expand All @@ -438,6 +427,27 @@ else if (element_ instanceof HtmlRadioButtonInput) {
return value;
}

private String isElementChecked(String lowerName) {
if ("checked".equals(lowerName)) {
if (element_ instanceof HtmlCheckBoxInput) {
return trueOrFalse(((HtmlCheckBoxInput) element_).isChecked());
}
else if (element_ instanceof HtmlRadioButtonInput) {
return trueOrFalse(((HtmlRadioButtonInput) element_).isChecked());
}
}
return null;
}

private String disabledJsCheck(String lowerName) {
if ("disabled".equals(lowerName)) {
if (element_ instanceof DisabledElement) {
return trueOrFalse(((DisabledElement) element_).isDisabled());
}
}
return null;
}

@Override
public String getDomAttribute(final String name) {
assertElementNotStale();
Expand Down
2 changes: 1 addition & 1 deletion src/test/java/org/openqa/selenium/htmlunit/junit/BrowserRunner.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,7 @@ public class BrowserRunner extends Suite {
* @throws Throwable If an exception occurs
*/
public BrowserRunner(final Class<WebTestCase> klass) throws Throwable {
super(klass, Collections.<Runner>emptyList());
super(klass, Collections.emptyList());

if (BrowserVersionClassRunner.containsTestMethods(klass)) {
final Set<String> browsers = WebDriverTestCase.getBrowsersProperties();
Expand Down