Skip to content

Conversation

mojwang
Copy link
Contributor

@mojwang mojwang commented Jan 27, 2017

@mojwang mojwang force-pushed the js-safari-tech-preview branch from ebd15a9 to d8357d4 Compare January 27, 2017 01:47
@titusfortner titusfortner added the C-nodejs JavaScript Bindings label Jan 30, 2017
@mojwang
Copy link
Contributor Author

mojwang commented Jan 31, 2017

@jleyba do you mind taking a look at this? thx in advance...

Copy link
Contributor

Choose a reason for hiding this comment

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

boolean, not Boolean

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated the PR with code review feedback

@mojwang mojwang force-pushed the js-safari-tech-preview branch from d8357d4 to bb72a01 Compare January 31, 2017 18:26
Copy link
Contributor

Choose a reason for hiding this comment

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

This will always be false. You don't set the capability on the top-level capabilities object.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As requested, the capability is not set on the top-level capabilities objects.

Copy link
Contributor

Choose a reason for hiding this comment

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

I meant you need to properly handle extracting options from the capabilities object.
For consistency with Java, you should end up with something alone the lines of

// Some details omitted for brevity

class Options {
  setTechnologyPreview(preview) {
    this.options_['technologyPreview'] = !!preview;
    return this;
  }
}

function useTechnologyPreview(obj) {
    if (obj instanceof Options) {
      return !!obj.options_['technologyPreview']
    }

    if (obj && typeof obj === 'object') {
      return !!obj['technologyPreview'];
    }

    return false;
}


class Driver extends webdriver.WebDriver {
  static createSession(opt_config, opt_flow) {
    let caps, exe;

    if (opt_config instanceof Options) {
      caps = opt_config.toCapabilities();

    } else {
      caps = opt_config || Capabilities.safari();
    }

    if (useTechnologyPreview(caps.get(OPTIONS_CAPABILITY_KEY))) {
      exe = SAFARIDRIVER_TECHNOLOGY_PREVIEW_EXE;
    }
    // ...
  }
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jleyba, ah gotcha! Pushing up an update in a few minutes...

@mojwang mojwang force-pushed the js-safari-tech-preview branch 2 times, most recently from add2ae2 to d249070 Compare February 9, 2017 08:17
@mojwang
Copy link
Contributor Author

mojwang commented Feb 13, 2017

@jleyba, does everything look okay or is there any more feedback in case I missed something?

@mojwang mojwang force-pushed the js-safari-tech-preview branch 2 times, most recently from f7463a9 to e26c37c Compare February 14, 2017 19:07
Copy link
Contributor

@jleyba jleyba left a comment

Choose a reason for hiding this comment

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

One more nit

Copy link
Contributor

Choose a reason for hiding this comment

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

options_ may be null. Need:

!!(o.options_ && o.options_[TECHNOLOGY_PREVIEW_OPTIONS_KEY]);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jleyba null-check included

Update CHANGES.md with note that SafariDriver JS bindings now supports Safari Technology Preview

Use correct Closure Compiler annotation for boolean parameters

Set technologyPreview capability on the top-level capabilities object

To be consistent with Java, set technologyPreview boolean in options object and properly extract the options object when creating a session

Add Closer Compiler annotations to useTechnologyPreview helper function

Null check the options_ object within useTechnologyPreview helper function
@mojwang mojwang force-pushed the js-safari-tech-preview branch from e26c37c to cee6b5c Compare February 14, 2017 20:46
@jleyba jleyba merged commit cb00152 into SeleniumHQ:master Feb 14, 2017
@mojwang mojwang deleted the js-safari-tech-preview branch February 14, 2017 21:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-nodejs JavaScript Bindings
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants