Nelson João Morais (:njasm) Reporter
	Description • 8 years ago

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.11; rv:47.0) Gecko/20100101 Firefox/47.0
Build ID: 20160604131506

Steps to reproduce:

According to http://w3c.github.io/webdriver/webdriver-spec.html#get-cookie
thre's a missing implementation in marionette to get a single cookie matching it by name, when supplied.

	Nelson João Morais (:njasm) Reporter
	Comment 1 • 8 years ago

If confirmed, one implementation solution would be:

1. add function "GetBy(name)" to : https://dxr.mozilla.org/mozilla-central/source/testing/marionette/cookies.js
2. add function to listener like getCookieByName(name) {...}
3. refactor : https://dxr.mozilla.org/mozilla-central/source/testing/marionette/driver.js#2121
 or add new specific endpoint/method to get a cookie by name.

	Nelson João Morais (:njasm) Reporter
	Comment 2 • 8 years ago

After thinking awhile about it, i think that point 1, probably shouldn't be implemented.

the new function getCookieByName, could call the already implemented getCookies in the listener, and loop through all the cookies to find the one that matches.

	Andreas Tolfsen ❲:ato❳
	Comment 3 • 8 years ago

The specification can be a bit wobbly to read in this case as the Get Cookie endpoint takes an optional URL parameter `name`, that if matching a cookie name amongst the associated cookies in the cookie store of the current browsing context’s active document, is supposed to return a single cookie in a set.

To clarify this I’ve submitted https://github.com/w3c/webdriver/pull/310 which breaks these into two distinct endpoints.  Additionally it contains a proposed breaking change to return an error when a named cookie isn’t found, instead of an empty list.

With regards to implementation: We should add a separate command in testing/marionette/driver.js called GeckoDriver#getNamedCookie that iterates over the cookies it gets back from the listener.  A possible optimisation is to add an IPC function in testing/marionette/listener.js that returns a single cookie, but this is unnecessary until it proves a performance issue.

tl;dr: Let’s add a new command, getNamedCookie, to testing/marionette/driver.js that fetches a list of cookies from the listener (yield this.listener.getCookies()), iterates over it until it finds a matching cookie, and either returns the cookie or an error.

Some additional work will need to be done in the Python client.

	Nelson João Morais (:njasm) Reporter
	Comment 4 • 8 years ago

I'm thinking in starting contributing to Mozilla codebase for a good first bug, do you think this one is good first bug, I could give it a shot.

I already hg cloned mozilla-central and successfully built Firefox.
my problemas are:
1. Don't find the procedures (step-by-step) on how to provide a patch for review.
2. Since this change have a need to refactor/update the Python client, and I've no experience with Python code, this could be a no go for you guys. (but I would like to give it a try too).

I'm an experience developer with PHP, C# and Golang, so I don't see huge problems with Python but without code reviewers/mentoring could be a huge step. since I still don't had the change to study the Python client codebase.

What's Your opinion?

	Andreas Tolfsen ❲:ato❳
	Comment 5 • 8 years ago

I think this is a good first bug.

You can find some getting started guides for mozilla-central development here:
https://mozilla-version-control-tools.readthedocs.io/en/latest/hgmozilla/index.html
https://wiki.mozilla.org/User:Mjzffr/New_Contributors

The changes you will need to make in the Python client are very limited.  The Marionette.get_cookie function (https://github.com/mozilla/gecko-dev/blob/master/testing/marionette/client/marionette_driver/marionette.py#L1804) will need to change from retrieving all cookies to calling the new command.

You can run the Marioentte Python tests, located in testing/marionette/harness/marionette/tests/unit with `./mach marionette-test [TEST…]`.  You can optionally use the flag `-v` and `-vv` to get verbose output from Gecko, such as logging.debug("…") statements.  To make the harness not swallow these, you will also have to use `--gecko-log -` to redirect the Gecko log to stdout.

	Andreas Tolfsen ❲:ato❳
	Comment 6 • 8 years ago

I forgot to add that there is some more information on debugging the Marionette server available on https://developer.mozilla.org/en-US/docs/Mozilla/QA/Marionette.

	Andreas Tolfsen ❲:ato❳
	Comment 7 • 8 years ago

I also think it would be a good idea to hold off implementing this until the specification changes have been accepted.

	Nelson João Morais (:njasm) Reporter
	Comment 8 • 8 years ago

Ok, thanks for the reply.

I'm gonna stay 'on hold' for this one, while keeping an eye on the pull request acceptance, meanwhile I'll have a look at those links. 

thanks.

	07yogeshgupta
	Comment 9 • 7 years ago

can i contribute? I'm new to open source.

	Titus Fortner
	Comment 10 • 7 years ago

This is implemented now, but it is returning an array of serialised cookies instead of just the serialised cookie data as per the spec.

	Henrik Skupin [:whimboo][⌚️UTC+1]
	Comment 11 • 7 years ago

(In reply to Andreas Tolfsen ‹:ato› from comment #7)
> I also think it would be a good idea to hold off implementing this until the
> specification changes have been accepted.

Was that the case already? Or do we still have to wait? Mind mentoring this bug, Andreas?

	Andreas Tolfsen ❲:ato❳
	Comment 12 • 7 years ago

The relevant changes have been accepted to the specification.  The command to implement is https://w3c.github.io/webdriver/webdriver-spec.html#dfn-get-named-cookie.

This involves adding a new command to testing/marionette/driver.js called getNamedCookie that takes one argument, name.  It should iterate over the return value from `yield this.listener.getCookies()` and find the cookie with the designated name.

The new command can be exposed in the Marionette protocol as "cookie:get" in GeckoDriver.prototype.commands (https://github.com/mozilla/gecko-dev/blob/master/testing/marionette/driver.js#L2750).  The relevant tests live in https://github.com/mozilla/gecko-dev/blob/master/testing/marionette/harness/marionette_harness/tests/unit/test_cookies.py.

	Aashay Singhal
	Comment 13 • 7 years ago

Is anyone working already on this bug? i am asking because i want to do this. i have read the comments and understood what is to be done. so, should i go ahead? 
P.S. : I am new to open source please guide me.

	Aashay Singhal
	Comment 14 • 7 years ago

Also, how do i run the testcases? i mean they are not written for the new function. so, how do i test if my function is giving correct output or not? i have written a function and i want to test it. Please help

	David Burns :automatedtester
	Comment 15 • 7 years ago

You will need to update tests to prove that the work you have done is corrent. In comment 12 there are the details for which lines you need to update. When you have built firefox with the command `./mach build` you can run the tests with `./mach marionette-test`

	Andreas Tolfsen ❲:ato❳
	Comment 16 • 7 years ago

Also see https://wiki.mozilla.org/User:Mjzffr/New_Contributors for how to contribute.

	Aashay Singhal
	Comment 17 • 7 years ago

i have added a function named getCookiesByName in driver.js which takes the cookies through getCookies function and checks for the particular cookie by name.
Also, i have added a test-case named test_new in test_cookies.py

	Henrik Skupin [:whimboo][⌚️UTC+1]
	Comment 18 • 7 years ago

Comment on attachment 8826298 [details] [diff] [review]
1282970v1.diff

Review of attachment 8826298 [details] [diff] [review]:
-----------------------------------------------------------------

After talking to you on IRC yesterday it's good to see the first changes coming in. Here two suggestions for you first:

* When the patch is not ready yet, don't ask for review but for feedback, or needinfo
* When you want to upload a patch please make use of mozreview. How to set it up is explained in detail under [1]

Mozreview will allow a much better experience in handling of review comments.

Otherwise you will find feedback comments inline.

Thanks.


[1] https://mozilla-version-control-tools.readthedocs.org/en/latest/mozreview/commits.html#mozreview-commits

::: testing/marionette/client/marionette_driver/marionette.py
@@ +2010,5 @@
>  
>          :param name: Name of cookie to get.
>          """
> +        body = {"name": name}
> +        return self._send_message("getCookiesByName",body)

This method is to get a single cookie by name, and not multiple ones. Please adjust the method name in driver.js.

@@ +2022,5 @@
>          :returns: A list of cookies for the current domain.
>          """
>          return self._send_message("getCookies", key="value" if self.protocol == 1 else None)
> +    # def get_cookies_by_name(self, name):
> +    #     return self._send_message("GetCookiesByName", key="value")

Please remove code which is not needed anymore.

::: testing/marionette/driver.js
@@ +2040,4 @@
>    resp.body = yield this.listener.getCookies();
>  };
>  
> +GeckoDriver.prototype.getCookiesByName = function*(cmd, resp) {

As mentioned for the other file, this method returns a single cookie and not multiple ones.

@@ +2041,5 @@
>  };
>  
> +GeckoDriver.prototype.getCookiesByName = function*(cmd, resp) {
> +  assert.content(this.context)
> +   let name = cmd.parameters.name;

Please assert that this parameter is a valid string. See other commands in how to do it.

@@ +2052,5 @@
> +      return t[a];
> +    }
> +  }
> +  return null;
> +}

I told you on IRC that you will need the `then()` function here. So please make use of it. It has to look like:

return this.listener.getCookies().then(cookies => {
  // loop over cookies to find the first matching entry
})

Instead of a for you better use the [some()](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/some) method which will stop after finding the first match.

Also please try to make use of understandable variable names to allow a faster understanding of the code, and also please try to match the coding style of this file.

@@ +2763,4 @@
>  }
>  
>  GeckoDriver.prototype.commands = {
> +  "getCookiesByName" :GeckoDriver.prototype.getCookiesByName, 

Place this next to the other cookie commands, as best sorted alphabetically. Also please remove the trailing white-space.

@@ +2847,4 @@
>  
>    "addon:install": GeckoDriver.prototype.installAddon,
>    "addon:uninstall": GeckoDriver.prototype.uninstallAddon,
> +

Please revert this.

::: testing/marionette/harness/marionette_harness/tests/unit/test_cookies.py
@@ +44,5 @@
> +        if cookies is None:
> +            self.assertEquals(0,1)
> +        else:
> +            self.assertEquals(0,0)
> +    

There is already test_should_get_cookie_by_name() which covers what we need. Something we would need is a case which raises a NoSuchCookie exception because the wanted cookie cannot be found.

::: testing/marionette/listener.js
@@ +1603,5 @@
> +    }
> +  }
> +
> +  return 0;
> +}

There is no need to add another method to listener.js. Just make re-use of this.listener.getCookies() in driver.js as I told you on IRC yesterday.

	David Burns :automatedtester
	Comment 19 • 7 years ago

pinging the bug for bugsahoy to update.

	Meltem Kilic
	Comment 20 • 7 years ago

Hi there,
I'm looking for a mentored bug to fix to get started with Mozilla's open source. Has this bug been resolved? If not would it be a beginner friendly bug? I'm currently learning how to deal with cookies using node and javascript.
Thank you

	Andreas Tolfsen ❲:ato❳
	Comment 21 • 7 years ago

I’m going to go ahead and make the decision that we won’t fix this on the grounds that geckodriver already implements the Get Named Cookie command for retrieving a single, named cookie.

Marionette does indeed return the full list of cookies, but this has no practical performance penalties.  So in the interest of not bloating the Marionette API, I’m closing this bug.

	Henrik Skupin [:whimboo][⌚️UTC+1]
	Comment 22 • 7 years ago

(In reply to Meltem Kilic from comment #20)
> I'm looking for a mentored bug to fix to get started with Mozilla's open
> source. Has this bug been resolved? If not would it be a beginner friendly
> bug? I'm currently learning how to deal with cookies using node and
> javascript.

Hi Meltem. Even this bug is wontfix now, there are lots of others.

Marionette contains both Javascript and Python code. Are you confident with both? Or what's your level of expertise? Knowing that would make it easier to propose a bug you could work on, and to improve your skills in that language.

	Meltem Kilic
	Comment 23 • 7 years ago

(In reply to Henrik Skupin (:whimboo) from comment #22)
> (In reply to Meltem Kilic from comment #20)
> > I'm looking for a mentored bug to fix to get started with Mozilla's open
> > source. Has this bug been resolved? If not would it be a beginner friendly
> > bug? I'm currently learning how to deal with cookies using node and
> > javascript.
> 
> Hi Meltem. Even this bug is wontfix now, there are lots of others.
> 
> Marionette contains both Javascript and Python code. Are you confident with
> both? Or what's your level of expertise? Knowing that would make it easier
> to propose a bug you could work on, and to improve your skills in that
> language.

Hi Henrik,

I'm right now comfortable with Javascript. Haven't touched Python yet as my focus was getting better at Javascript since I seriously started on programming 3 months ago. I can make use of all the main functionalities of Javascript and looking forward to improve my skills through real-life applications of the language. Given that I would highly appreciate if you could propose a bug that I could work on. Feel free to checkout my github profile here: https://github.com/turquoisemelon. As a junior in the Web Development field, I'm learning a lot of new stuff each day, and sometimes find it hard to position my level of expertise.

Thank you.

	Nelson João Morais (:njasm) Reporter
	Comment 24 • 7 years ago

(In reply to Andreas Tolfsen ‹:ato› from comment #21)
> I’m going to go ahead and make the decision that we won’t fix this on the
> grounds that geckodriver already implements the Get Named Cookie command for
> retrieving a single, named cookie.
> 
> Marionette does indeed return the full list of cookies, but this has no
> practical performance penalties.  So in the interest of not bloating the
> Marionette API, I’m closing this bug.

I know I'm late on the issue.
And I surely can understand the argument of not "bloating the Marionette API" but doesn't this argument goes against the premise that marionette will/want to implement the webdriver spec or at least comply with it?

As someone developing a client for marionette (And I know that Marionette is not intended for that, for that we have geckodriver through selenium) its hard to understand the direction of marionette's project.

	Andreas Tolfsen ❲:ato❳
	Comment 25 • 7 years ago

(In reply to Nelson João Morais (:njasm) from comment #24)

> (In reply to Andreas Tolfsen ‹:ato› from comment #21)
> 
> > I’m going to go ahead and make the decision that we won’t fix
> > this on the grounds that geckodriver already implements the Get
> > Named Cookie command for retrieving a single, named cookie.
> > 
> > Marionette does indeed return the full list of cookies, but this
> > has no practical performance penalties.  So in the interest of not
> > bloating the Marionette API, I’m closing this bug.
> 
> I know I'm late on the issue.  And I surely can understand the
> argument of not "bloating the Marionette API" but doesn't this
> argument goes against the premise that marionette will/want to
> implement the webdriver spec or at least comply with it?

Marionette does not directly implement WebDriver. geckodriver will
provide an implementation of the Get Named Cookie command based on the
getCookies Marionette command, so our WebDriver implementation will be
compliant.

> As someone developing a client for marionette (And I know that
> Marionette is not intended for that, for that we have geckodriver
> through selenium) its hard to understand the direction of marionette's
> project.

It is absolutely intended that clients can speak directly to Marionette,
but you need to be aware that Marionette is the Gecko remote protocol
and not an implementation of WebDriver. geckodriver _uses_ the remote
protocol to implement WebDriver.

Please let me know if this helps, otherwise I can try to explain better.