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.

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

Implement EventFiringWebDriver and EventFiringWebElement #76

Merged
merged 7 commits into from Sep 4, 2013

Conversation

Projects
None yet
2 participants
*
* @return EventFiringWebDriver
*/
// protected function back() {

This comment has been minimized.

@whhone

whhone Aug 22, 2013

Contributor

WebDriver is not going to implement this method. To go back to the previous page,

  $driver->navigate()->back();

It is the same for forward().

$this->_dispatch('beforeFindBy', $by);
$raw_element = $this->executor->execute('elementFindElement', [

This comment has been minimized.

@whhone

whhone Aug 22, 2013

Contributor

How about $element = $this->element->findElement($by) and return $element?

The same for findElements.

* @param string $id
* @param WebDriverDispatcher $dispatcher
*/
public function __construct(WebDriverCommandExecutor $executor, $id, WebDriverDispatcher $dispatcher = null) {

This comment has been minimized.

@whhone

whhone Aug 22, 2013

Contributor

Construct it by just WebDriverElement instead of $executor and $id.

@whhone

This comment has been minimized.

Copy link
Contributor

whhone commented Aug 22, 2013

I think we don't need WebDriverDispatcher.php. Also, to register a listener, we should do

$eventFiringWebDriver->register(new SeleniumListener());

instead of

$eventFiringWebDriver->getDispatcher()->register(new SeleniumListener());

in your example.

Implemented EventFiringWebDriverNavigation to use the navigation events.
Where applicable, using the underlying element item to find elements.

Abstract class EventFiringObject added to encapsulate some of the generic methods and dispatcher.
@ashleydw

This comment has been minimized.

Copy link
Contributor

ashleydw commented Aug 22, 2013

You're correct about the example, I changed the dispatcher code after posting that comment.

With regards to WebDriverDispatcher.php, what do you suggest?

@whhone

This comment has been minimized.

Copy link
Contributor

whhone commented Aug 22, 2013

In the Java binding, dispatcher is an instance of WebDriverEventListener. https://github.com/SeleniumHQ/selenium/blob/master/java/client/src/org/openqa/selenium/support/events/EventFiringWebDriver.java#L70

Also, try not to use __call(). This is a magical function but it is very hard to debug when anything goes wrong. It is one of the reason why we need to rewrite the binding.

Making protected methods into public.
Removed EventFiringObject.
many try/catch statements is a compromise
@ashleydw

This comment has been minimized.

Copy link
Contributor

ashleydw commented Aug 23, 2013

The Java binding is running it as a proxy instance, I'm unaware of a method similar in php which allows this. Of course the WebDriverEventListener and WebDriverDispatcher could be merged into one class, but personally I prefer the separation of the two.

Updated protected methods to public each with a try/catch - not as pretty but you're correct in that it provided better debugging/ide support.

@@ -0,0 +1,89 @@
<?php

This comment has been minimized.

@whhone

whhone Aug 28, 2013

Contributor

Please add the license header here.
Also, it should be an interface instead of abstract class.

@@ -35,6 +35,10 @@ public function __construct(
);
}
public function getExecutor() {

This comment has been minimized.

@whhone

whhone Aug 28, 2013

Contributor

Remove this method.

@@ -0,0 +1,351 @@
<?php

This comment has been minimized.

@whhone

whhone Aug 28, 2013

Contributor

license header

@whhone

This comment has been minimized.

Copy link
Contributor

whhone commented Aug 28, 2013

Looks good to be merged. Please add the license header to new files.

Sorry for the delay because your pull requests exposed a design issue on the currect php-webdriver. We will resolve it later. :D

Licensing and WebDriverEventListener is an interface.
Also simple phpdoc changes for exceptions
@ashleydw

This comment has been minimized.

Copy link
Contributor

ashleydw commented Sep 4, 2013

Added licenses, and changed the abstract class to an interface

@whhone

This comment has been minimized.

Copy link
Contributor

whhone commented Sep 4, 2013

We can’t automatically merge this pull request. Can you do a rebase?

@ashleydw

This comment has been minimized.

Copy link
Contributor

ashleydw commented Sep 4, 2013

I have rebased, with a conflict in init. Wasn't sure if you wanted it in a new directory as the other refactored code.

All running fine in my local selenium tests. She's all yours:)

whhone added a commit that referenced this pull request Sep 4, 2013

Merge pull request #76 from ashleydw/master
Implement EventFiringWebDriver and EventFiringWebElement

@whhone whhone merged commit dfefb6d into facebook:master Sep 4, 2013

@whhone

This comment has been minimized.

Copy link
Contributor

whhone commented Sep 4, 2013

Thank you! I will tidy up it a little bit for the coding style and structure of those new classes.

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