Issue #228: Allowed the setting of custom headers

[MorrisonCole]

This pull-request allows you to pass custom request headers down to PhantomJS. The syntax for doing so is a little different to the PhantomJS equivalent, but is relatively simple:

In PhantomJS, you might do something like:

var page = require('webpage').create();
page.customHeaders = {
  'Accept-Language': 'it-IT',
  'Max-Forwards': '6'
};

And (with this patch), you can do:

capabilities.setCapability(PHANTOMJS_PAGE_CUSTOMHEADERS_PREFIX + "Accept-Language", "it-IT");
capabilities.setCapability(PHANTOMJS_PAGE_CUSTOMHEADERS_PREFIX + "Max-Forwards", "6");

Similar resolved issues: Issue #134

Technical notes:
After the iterations on the implementation, we've ended up with a bit of duplication. My Javascript skills are sufficiently bad that I'm not going to refactor it without the tools to test afterward (latest commit is from a dying laptop with no charger), so I'll leave that in for now. To be fair, it reads fine anyway.

Also, it'd be nice to update the page.customHeaders object so that we don't lose any previous key/values that may have existed. Not really a problem, as it doesn't look like we have any default headers at the moment anyway, but it might need to be changed in the future.

[detro]

I think the actual string is wrong here.
Also, I'd call it PHANTOMJS_PAGE_CUSTOMHEADERS_PREFIX.

[detro]

Could you please add a bit more explaination to the PR?
I do get what you want to do reading your code, but a verbal explaination would make sure it does what it says on the tin.

BTW, thank you very much for this effort.
I'm sure this is going to be merged once those small issues are addressed ;)

[MorrisonCole]

@detro I've updated the pull-request with an example usage and (basic) explanation.

Also note that I changed:

PHANTOMJS_CUSTOMHEADERS_PREFIX = "phantomjs.customHeaders"

to:

PHANTOMJS_PAGE_CUSTOMHEADERS_PREFIX = "phantomjs.page.customHeaders"

(per your suggestion).

[detro]

I'm stupid.
I missed the fact that the way this works is by setting 1 Capability and a list of HEADER:VALUE,HEADER:VALUE... headers.

This doesn't match the way "phantomjs.page.settings.x" works: that requires you to set 1 Capability PER setting.
So something like:

setCapability("phantomjs.page.settings.userAgent", "My user agent");
setCapability("phantomjs.page.settings.AllowXSS", true);

and so forth.

That's why I called it PHANTOMJS_PAGE_SETTINGS_PREFIX: because that constant only provides a PREFIX and the developer has to use that and attach another string to it with the name of the setting they want to set.

I'd like to maintain consistency: could you please refactor?

Btw, the changes you have done are all going in the right direction.
Sorry if I seem a bit "anal" :)

[MorrisonCole]

I'd argue that it'd be more consistent implementing it in a similar way to PhantomJS.

If in Phantom we're doing

page.customHeaders = { 'Accept-Language': 'it-IT' };

Shouldn't GhostDriver be doing:

capabilities.setCapability("phantomjs.page.customHeaders", "Accept-Language:it-IT");

Of course if you still don't want to do that, I'm happy to change it (although I don't think it makes much sense).

[detro]

Leaving the way you suggest will make 2 capabilities, that do 2 very (very!) similar things, require a totally different approach.

And I can't change phantomjs.page.settings now because it's already released and I don't think it's worth to pay the price of breaking compatibility.

It's a tool, and people don't like when you break their tools :)

[MorrisonCole]

Point taken. I personally preferred sticking as closely as possible to the PhantomJS implementation, but it does make it a little messy (comma delimited argument passing, eurgh).

So before I go ahead and make the changes, you want something like:

PHANTOMJS_PAGE_CUSTOMHEADERS_PREFIX = "phantomjs.page.customHeaders.";
capabilities.setCapability(PHANTOMJS_PAGE_CUSTOMHEADERS_PREFIX + "Accept-Language", "it-IT");

Right? :)

Edit: Facepalm, corrected to PHANTOMJS_PAGE_CUSTOMHEADERS_PREFIX, was tired!

[detro]

PHANTOMJS_PAGE_CUSTOMHEADERS_PREFIX = "phantomjs.page.customHeaders."; //< notice the dot at the end
capabilities.setCapability(PHANTOMJS_PAGE_CUSTOMHEADERS_PREFIX + "Accept-Language", "it-IT");

[MorrisonCole]

Done and done. Updated the pull-request description to reflect the changes.

I agree that it's nicer as you suggested. Cool!

[detro]

Merged.
Thanks!