Closed Bug 1371733 Opened 7 years ago Closed 7 years ago

Move cookie service to chrome

Categories

(Remote Protocol :: Marionette, enhancement)

Version 3
enhancement
Not set
normal

Tracking

(firefox56 fixed)

RESOLVED FIXED
mozilla56
Tracking Status
firefox56 --- fixed

People

(Reporter: ato, Assigned: ato)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

Attachments

(5 files, 1 obsolete file)

Marionette provides a cookie service to the content frame script that allows cookies to be queried from chrome.  This is unnecessary since the only consumer of this is the client, and one of the few things chrome does know about the content browser, is its document URL.
Good to know. So lets block bug 1368439 for this.
Blocks: 1368439
Assignee: nobody → ato
Status: NEW → ASSIGNED
Attachment #8876792 - Flags: review?(hskupin)
Comment on attachment 8876786 [details]
Bug 1371733 - Move cookie service to chrome space;

https://reviewboard.mozilla.org/r/148110/#review152806

You didn't set a review yet, but here a short notice of how to update the driver.js. Maybe you were already waiting for it to get merged to m-c.

::: testing/marionette/driver.js:2470
(Diff revision 3)
> -GeckoDriver.prototype.deleteAllCookies = function* (cmd, resp) {
> +GeckoDriver.prototype.deleteAllCookies = function (cmd, resp) {
>    assert.content(this.context);
>    assert.window(this.getCurrentWindow());
>    assert.noUserPrompt(this.dialog);
>  
> -  let cb = msg => {
> +  const currentURI = this.curBrowser.contentBrowser.currentURI;

Please use the logic as implemented via bug 1368492 for each of the methods.
(In reply to Henrik Skupin (:whimboo) from comment #11)

> You didn't set a review yet, but here a short notice of how to update the
> driver.js. Maybe you were already waiting for it to get merged to m-c.
> 
> ::: testing/marionette/driver.js:2470
> > +  const currentURI = this.curBrowser.contentBrowser.currentURI;
> 
> Please use the logic as implemented via bug 1368492 for each of the methods.

Yes, I wrote this before https://bugzilla.mozilla.org/show_bug.cgi?id=1368492
landed.  I will update the patch to use that before requesting review.  Thanks
for reminding me.
I filed https://bugzilla.mozilla.org/show_bug.cgi?id=1372582 as a
follow-up bug to make the cookie service WebDriver conforming.  I have
somewhat intentionally made very small strides towards conformance in
this patch.
Blocks: 1372582
Comment on attachment 8878453 [details]
Bug 1371733 - Map ato's email address;

https://reviewboard.mozilla.org/r/149804/#review155316

::: .mailmap:1
(Diff revision 1)
> +Andreas Tolfsen <ato@sny.no> <ato@mozilla.com>

I don't think this is a file you want to checkin.
Comment on attachment 8877151 [details]
Bug 1371733 - Encode currentURL as URL object;

https://reviewboard.mozilla.org/r/148498/#review155318

::: commit-message-a8f8e:3
(Diff revision 2)
> +Bug 1371733 - Encode currentURL as URL object; r?whimboo
> +
> +Instead of returning a string representation of the current locaton from

s/locaton/location.

::: testing/marionette/driver.js:155
(Diff revision 2)
> +/**
> + * Returns the current URL of the ChromeWindow or content browser,
> + * depending on context.
> + *
> + * @return {URL}
> + *     Read-only property containing the currently loaded URL.

If we have to call out `readonly` it should then be done in the description of the property but not for the return value.
Attachment #8877151 - Flags: review?(hskupin) → review+
Comment on attachment 8876781 [details]
Bug 1371733 - Singularise cookie module name;

https://reviewboard.mozilla.org/r/148106/#review155322
Attachment #8876781 - Flags: review?(hskupin) → review+
Comment on attachment 8876782 [details]
Bug 1371733 - Propagate error message of assert.object;

https://reviewboard.mozilla.org/r/148108/#review155324
Attachment #8876782 - Flags: review?(hskupin) → review+
Comment on attachment 8876792 [details]
Bug 1371733 - Add tests for custom assertion errors;

https://reviewboard.mozilla.org/r/148116/#review155326

::: commit-message-f4f6a:1
(Diff revision 4)
> +Bug 1371733 - Add tests for custom assertion errors; r?whimboo

As best combine both commits. I don't see why we need two differnt ones.
Attachment #8876792 - Flags: review?(hskupin) → review+
Comment on attachment 8876786 [details]
Bug 1371733 - Move cookie service to chrome space;

https://reviewboard.mozilla.org/r/148110/#review155536

::: testing/marionette/cookie.js:20
(Diff revision 5)
>  
>  const IPV4_PORT_EXPR = /:\d+$/;
>  
> -/**
> - * Interface for manipulating cookies from content space.
> - */
> +this.cookie = {
> +  manager: Services.cookies,
> +};

You are not going to use a class here? What is the reason?

::: testing/marionette/cookie.js:22
(Diff revision 5)
> -/**
> - * Interface for manipulating cookies from content space.
> - */
> +this.cookie = {
> +  manager: Services.cookies,
> +};
> -this.Cookies = class {
>  
> -  /**
> +cookie.fromJSON = function (json) {

If this should be public please add the documenation.

::: testing/marionette/cookie.js:58
(Diff revision 5)
> -   * @param {Object.<string, ?>} opts
> -   *     An object with the optional fields {@code domain}, {@code path},
> -   *     {@code secure}, {@code httpOnly}, and {@code expiry}.
> -   *
> + *
> -   * @return {Object.<string, ?>}
> -   *     A serialisation of the cookie that was added.
> + *       restrictToHost (string)
> + *         Perform test that |newCookie|'s domain matches this.

I don't like the usage of `host` vs `domain` across this file. We should use one or the other.

::: testing/marionette/cookie.js:68
(Diff revision 5)
> -   *     otherwise was issues with the input data.
> + * @throws {InvalidCookieDomainError}
> + *     If |restrictToHost| is set and |newCookie|'s domain does not match.
> -   */
> + */
> -  add(name, value, opts={}) {
> -    if (typeof this.document == "undefined" || !this.document.contentType.match(/html/i)) {
> -      throw new UnableToSetCookieError(
> +cookie.add = function (newCookie, opts = {}) {
> +  if (typeof newCookie.name != "string") {
> +    throw new TypeError("Cookie must have string name");

Please use the same wording for all three asserts as you did above for `fromJSON`.

::: testing/marionette/cookie.js:96
(Diff revision 5)
>  
> -    if (!opts.domain) {
> +    if (newCookie.domain !== opts.restrictToHost) {
> -      opts.domain = this.document.location.host;
> -    } else if (this.document.location.host.indexOf(opts.domain) < 0) {
>        throw new InvalidCookieDomainError(
> -          "You may only set cookies for the current domain");
> +          "Cookies may only be set for the current domain");

Maybe we should add the value of the domain for a better error message.

::: testing/marionette/cookie.js:114
(Diff revision 5)
> -      value: value,
> -      secure: opts.secure,
> -      httpOnly: opts.httpOnly,
> -      session: false,
> -      expiry: opts.expiry,
> +      newCookie.value,
> +      newCookie.secure,
> +      newCookie.httpOnly,
> +      newCookie.session,
> +      newCookie.expiry,
> +      {} /* origin attributes */);

I do not see such an additional parameter for `add` on MDN: https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Interface/nsICookieManager2#add()

::: testing/marionette/cookie.js:117
(Diff revision 5)
> -    };
> +};
> -    if (!this.chrome.addCookie(cookie)) {
> -      throw new UnableToSetCookieError();
> -    }
>  
> -    return cookie;
> +cookie.remove = function (toDelete) {

Missing documenation. Please also check all the other methods.

::: testing/marionette/cookie.js:129
(Diff revision 5)
> -      name = cookie.name;
> -    } else {
> -      name = cookie;
> -    }
>  
> -    for (let candidate of this) {
> +  let en = cookie.manager.getCookiesFromHost(host, {});

There is no second argument.

::: testing/marionette/cookie.js:133
(Diff revision 5)
>  
> -    for (let candidate of this) {
> -      if (candidate.name == name) {
> -        if (!this.chrome.deleteCookie(candidate)) {
> -          throw new UnknownError("Unable to delete cookie by name: " + name);
> -        }
> +  let en = cookie.manager.getCookiesFromHost(host, {});
> +  while (en.hasMoreElements()) {
> +    let cookie = en.getNext().QueryInterface(Ci.nsICookie2);
> +    // take the hostname and progressively shorten
> +    let hostname = host;

Use an assert to make sure `host` is a string.

::: testing/marionette/cookie.js:145
(Diff revision 5)
> +          "path": cookie.path,
> +          "domain": cookie.host,
> +          "secure": cookie.isSecure,
> +          "httpOnly": cookie.isHttpOnly,
> +          "expiry": cookie.expires,
> +        };

There is no need for `break` here due to the `yield` statement? Just want to make sure that we don't continue to yield even an entry was found.

::: testing/marionette/driver.js:2410
(Diff revision 5)
>    assert.noUserPrompt(this.dialog);
>  
> -  let cb = msg => {
> -    this.mm.removeMessageListener("Marionette:addCookie", cb);
> -    let cookie = msg.json;
> -    Services.cookies.add(
> +  let {protocol, hostname} = this.currentURL;
> +
> +  const networkSchemes = ["ftp:", "http:", "https:"];
> +  if (!networkSchemes.includes(protocol)) {

Shouldn't we better do this check in cookie.js itself?

::: testing/marionette/listener.js
(Diff revision 5)
>    addMessageListenerId("Marionette:getAppCacheStatus", getAppCacheStatus);
>    addMessageListenerId("Marionette:takeScreenshot", takeScreenshotFn);
> -  addMessageListenerId("Marionette:addCookie", addCookieFn);
> -  addMessageListenerId("Marionette:getCookies", getCookiesFn);
> -  addMessageListenerId("Marionette:deleteAllCookies", deleteAllCookiesFn);
> -  addMessageListenerId("Marionette:deleteCookie", deleteCookieFn);

Hurray for clean-up!

::: testing/marionette/test_cookie.js:21
(Diff revision 5)
> +      value,
> +      secure,
> +      httpOnly,
> +      session,
> +      expiry,
> +      originAttributes) {

Really weird formatting, and not quickly to understand. I have a dislike of this by not following the general JS style guide for argument wrapping.

::: testing/marionette/test_cookie.js:247
(Diff revision 5)
> +
> +  cookie.add({name: "0", value: "", domain: "example.com"});
> +  cookie.add({name: "1", value: "", domain: "foo.example.com"});
> +  cookie.add({name: "2", value: "", domain: "bar.example.com"});
> +
> +  let fooCookies = [...cookie.iter("foo.example.com")];

Can you please also add a test for iterating with `.com` as domain? I want to make sure that the yield is working correctly as pointed out above.

::: testing/marionette/unit.ini:12
(Diff revision 5)
>  [DEFAULT]
>  skip-if = appname == "thunderbird"
>  
>  [test_action.js]
>  [test_assert.js]
> +[test_cookie.js]

Shouldn't we also add a wdspec test? I would be happy to do it as a separate and mentored bug. Please file.
Attachment #8876786 - Flags: review?(hskupin) → review-
Comment on attachment 8878453 [details]
Bug 1371733 - Map ato's email address;

https://reviewboard.mozilla.org/r/149804/#review155316

> I don't think this is a file you want to checkin.

I do want to check that in.
Comment on attachment 8877151 [details]
Bug 1371733 - Encode currentURL as URL object;

https://reviewboard.mozilla.org/r/148498/#review155318

> If we have to call out `readonly` it should then be done in the description of the property but not for the return value.

I copied this from the IDL file for nsIURI, but I can drop “read-only” as it bears no significance here.
Comment on attachment 8876792 [details]
Bug 1371733 - Add tests for custom assertion errors;

https://reviewboard.mozilla.org/r/148116/#review155326

> As best combine both commits. I don't see why we need two differnt ones.

This commit adds tests for custom error messages for all other assertions so they don’t regress.  They’re not related to the change fix for assert.object that I am making in the previous commit.
Comment on attachment 8878453 [details]
Bug 1371733 - Map ato's email address;

https://reviewboard.mozilla.org/r/149804/#review155316

> I do want to check that in.

Then request review from Gregory for that commit please.
Comment on attachment 8876786 [details]
Bug 1371733 - Move cookie service to chrome space;

https://reviewboard.mozilla.org/r/148110/#review155536

> If this should be public please add the documenation.

I found I don’t need to attach any special behaviour to the cookie itself, so there’s no need for a class.  This is really just validating the cookie according to what a cookie should look like according to the spec.

> I don't like the usage of `host` vs `domain` across this file. We should use one or the other.

We need to keep this distinction as ‘domain’ is the WebDriver property used in the cookie representation, ‘host’ is what is used by the cookie layer, and ‘hostname’ is the correct terminology according to the URL spec.

> I do not see such an additional parameter for `add` on MDN: https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Interface/nsICookieManager2#add()

https://searchfox.org/mozilla-central/rev/2bcd258281da848311769281daf735601685de2d/netwerk/cookie/nsICookieManager2.idl#49-50

> Missing documenation. Please also check all the other methods.

The primary reason I didn’t document cookie.remove is because it is self-explanatory, and the reason I didn’t document cookie.iter is because I don’t really understand what it is doing… I can add some skeleton documentation though.

> There is no second argument.

https://searchfox.org/mozilla-central/rev/2bcd258281da848311769281daf735601685de2d/netwerk/cookie/nsICookieManager2.idl#131-132

> Use an assert to make sure `host` is a string.

Just to make it clear: This is not code I wrote, but I can add a type check.

> There is no need for `break` here due to the `yield` statement? Just want to make sure that we don't continue to yield even an entry was found.

A yield returns an iterator object behind the scenes which has a getNext() function.  It is only called if there the next entry exists.

> Shouldn't we better do this check in cookie.js itself?

I did consider that, but the notion of ‘cookie-averse’ documents is a WebDriver concept and not inherently one that the cookie service has.  I’m quite sure that if you try to add a cookie for an about: document, then nsICookieManager2 will throw its own exception.  In other words, this is a specialisation for WebDriver.

> Really weird formatting, and not quickly to understand. I have a dislike of this by not following the general JS style guide for argument wrapping.

What does “the general JS style guide” say about this?  I don’t understand what you want.

> Can you please also add a test for iterating with `.com` as domain? I want to make sure that the yield is working correctly as pointed out above.

In short, no.  I’m not sure cookie.iter handles that and I don’t want to change it.

> Shouldn't we also add a wdspec test? I would be happy to do it as a separate and mentored bug. Please file.

I’m adding a WPT test in https://bugzilla.mozilla.org/show_bug.cgi?id=1372595.
Comment on attachment 8876786 [details]
Bug 1371733 - Move cookie service to chrome space;

https://reviewboard.mozilla.org/r/148110/#review155536

> You are not going to use a class here? What is the reason?

The description in the other issue is plausible. Thanks.

> I found I don’t need to attach any special behaviour to the cookie itself, so there’s no need for a class.  This is really just validating the cookie according to what a cookie should look like according to the spec.

I don't see that this is the answer for this issue. So reopening it.

> We need to keep this distinction as ‘domain’ is the WebDriver property used in the cookie representation, ‘host’ is what is used by the cookie layer, and ‘hostname’ is the correct terminology according to the URL spec.

*Shrug*... ok.

> Please use the same wording for all three asserts as you did above for `fromJSON`.

I don't see that this has been fixed in the latest revision. Maybe you missed to push it?

> Maybe we should add the value of the domain for a better error message.

Same here regarding a missing update in the patch.

> https://searchfox.org/mozilla-central/rev/2bcd258281da848311769281daf735601685de2d/netwerk/cookie/nsICookieManager2.idl#49-50

Never trust MDN, and better check the interfaces directly! :/ Thanks.

> The primary reason I didn’t document cookie.remove is because it is self-explanatory, and the reason I didn’t document cookie.iter is because I don’t really understand what it is doing… I can add some skeleton documentation though.

Reopening due to missing changes.

> Just to make it clear: This is not code I wrote, but I can add a type check.

You requested that in the past too when I was updating code I haven't written. I don't think it's troublesome to add, but makes us way better in handling invalid argument values.

Also reopening issue due to missing changes in the last patch updates.

> A yield returns an iterator object behind the scenes which has a getNext() function.  It is only called if there the next entry exists.

Ok, so to clarify it returns immediately so that no other code afterward is getting executed? If yes, please drop the issue.

> I did consider that, but the notion of ‘cookie-averse’ documents is a WebDriver concept and not inherently one that the cookie service has.  I’m quite sure that if you try to add a cookie for an about: document, then nsICookieManager2 will throw its own exception.  In other words, this is a specialisation for WebDriver.

Ok, fine with me.

> What does “the general JS style guide” say about this?  I don’t understand what you want.

As we already discussed in the past, there is no concept for 4 char indendation, but I'm not going to argue about this again.

Second if the function argument list is wrapped it should start at the same column as the the line before:

```
add: function (domain, path, ....
               name,
```

Also I don't see why putting all args in a separate line makes sense here in the test.

> In short, no.  I’m not sure cookie.iter handles that and I don’t want to change it.

So how is the while loop in `cookie.iter` going to be tested if it doesn't match immediately but needs at least another cycle?

> I’m adding a WPT test in https://bugzilla.mozilla.org/show_bug.cgi?id=1372595.

Ok, but that might only cover the case for a single cookie. Here you are refactoring the whole cookie implementation.
Comment on attachment 8876786 [details]
Bug 1371733 - Move cookie service to chrome space;

https://reviewboard.mozilla.org/r/148110/#review156202
Attachment #8876786 - Flags: review?(hskupin) → review-
Comment on attachment 8876786 [details]
Bug 1371733 - Move cookie service to chrome space;

https://reviewboard.mozilla.org/r/148110/#review155536

> I don't see that this is the answer for this issue. So reopening it.

I discovered that I had somehow forgotten to commit the documentation
change.  It should be in the patch now.

> I don't see that this has been fixed in the latest revision. Maybe you missed to push it?

I think I must have.  Sorry about that.

> Ok, so to clarify it returns immediately so that no other code afterward is getting executed? If yes, please drop the issue.

> Ok, so to clarify it returns immediately so that no other code
> afterward is getting executed? If yes, please drop the issue.

Right, 'yield' returns from this function with the inner state necessary
to perform iteration.  No code after it will be executed.  This is
comparable to generator functions in Python.

> As we already discussed in the past, there is no concept for 4 char indendation, but I'm not going to argue about this again.
> 
> Second if the function argument list is wrapped it should start at the same column as the the line before:
> 
> ```
> add: function (domain, path, ....
>                name,
> ```
> 
> Also I don't see why putting all args in a separate line makes sense here in the test.

> Second if the function argument list is wrapped it should start at the
> same column as the the line before:

This is a convention we don’t (currently) use in Marionette, but is
also one I don’t care much for.  Visual indentation like this only
really works if you use monospaced fonts and since we don’t have any
tools to automatically format the code, it significantly increases the
time I need to spend on formalities that doesn’t impact the behaviour
of the code.

In an effort to land this code, I have put all the arguments on one
single line.  I hope this is acceptable to you.

> So how is the while loop in `cookie.iter` going to be tested if it doesn't match immediately but needs at least another cycle?

> So how is the while loop in cookie.iter going to be tested if it
> doesn't match immediately but needs at least another cycle?

I don’t know the answer to that question, but I don’t think this
should be blocking the move of the cookie service to chrome space.

> Ok, but that might only cover the case for a single cookie. Here you are refactoring the whole cookie implementation.

> Ok, but that might only cover the case for a single cookie. Here you
> are refactoring the whole cookie implementation.

Right, I’m refactoring it.  Not actually addressing conformance issues.
Attachment #8878453 - Attachment is obsolete: true
Comment on attachment 8876786 [details]
Bug 1371733 - Move cookie service to chrome space;

https://reviewboard.mozilla.org/r/148110/#review155536

> > Ok, so to clarify it returns immediately so that no other code
> > afterward is getting executed? If yes, please drop the issue.
> 
> Right, 'yield' returns from this function with the inner state necessary
> to perform iteration.  No code after it will be executed.  This is
> comparable to generator functions in Python.

Well, I just checked again, and you can have multiple yields in a row within the same generator. It means that it is not an immediate return! Here an example:

```
function* iter(host, hostname) {
   let found = false;
   do {
      if ((host == "." + hostname || host == hostname)) {
        yield hostname;
        found = true;
        // break;
      }
      alert(found);     
      hostname = hostname.replace(/^.*?\./, "");
   } while (hostname.indexOf(".") != -1);
}

let gen = iter("example.org", "www.vhost.example.org");
alert(gen.next().value);
alert(gen.next().value);
alert(gen.next().value);
```

You should never get an alert with `true`. But you do with your code. It won't hurt that much because we only yield once when it equals, but we unnecessarily would continue to loop until all dots have been exhausted. To save this time lets add a `break`, which I commented out above.

> > So how is the while loop in cookie.iter going to be tested if it
> > doesn't match immediately but needs at least another cycle?
> 
> I don’t know the answer to that question, but I don’t think this
> should be blocking the move of the cookie service to chrome space.

You can vary `host` and `hostname` like I do above in my example for the generator code. Just adding some variations should be fine, right?

> > Ok, but that might only cover the case for a single cookie. Here you
> > are refactoring the whole cookie implementation.
> 
> Right, I’m refactoring it.  Not actually addressing conformance issues.

Well, so lets defer then.
Comment on attachment 8876786 [details]
Bug 1371733 - Move cookie service to chrome space;

https://reviewboard.mozilla.org/r/148110/#review156746

::: testing/marionette/cookie.js:87
(Diff revisions 7 - 9)
>   * @throws {InvalidCookieDomainError}
>   *     If |restrictToHost| is set and |newCookie|'s domain does not match.
>   */
>  cookie.add = function (newCookie, opts = {}) {
>    if (typeof newCookie.name != "string") {
> -    throw new TypeError("Cookie must have string name");
> +    throw new TypeError("Cookie name must be string");

Err, the spec says we have to return an `InvalidArgumentError`. So why not using `assert.string()` in all those cases when the key exists?

::: testing/marionette/cookie.js:166
(Diff revisions 7 - 9)
> + *
> + * @return {[Symbol.Iterator]}
> + *     Iterator.
> + */
>  cookie.iter = function* (host, currentPath = "/") {
> -  let isForCurrentPath = path => currentPath.indexOf(path) != -1;
> +  assert.string(host);

Please add the message.
Attachment #8876786 - Flags: review?(hskupin) → review-
Comment on attachment 8876786 [details]
Bug 1371733 - Move cookie service to chrome space;

https://reviewboard.mozilla.org/r/148110/#review155536

> You requested that in the past too when I was updating code I haven't written. I don't think it's troublesome to add, but makes us way better in handling invalid argument values.
> 
> Also reopening issue due to missing changes in the last patch updates.

I have added an assertion in the latest revision of the patch.

> Well, I just checked again, and you can have multiple yields in a row within the same generator. It means that it is not an immediate return! Here an example:
> 
> ```
> function* iter(host, hostname) {
>    let found = false;
>    do {
>       if ((host == "." + hostname || host == hostname)) {
>         yield hostname;
>         found = true;
>         // break;
>       }
>       alert(found);     
>       hostname = hostname.replace(/^.*?\./, "");
>    } while (hostname.indexOf(".") != -1);
> }
> 
> let gen = iter("example.org", "www.vhost.example.org");
> alert(gen.next().value);
> alert(gen.next().value);
> alert(gen.next().value);
> ```
> 
> You should never get an alert with `true`. But you do with your code. It won't hurt that much because we only yield once when it equals, but we unnecessarily would continue to loop until all dots have been exhausted. To save this time lets add a `break`, which I commented out above.

My earlier explanation that “it returns immediately” did not mean to
imply that it is analogous to returning a value.  I’m sorry if this
was misleading.

A generator function, identified by "*" and a yield statement,
provides a so called ‘iterator object’ or [Symbol.Iterator]
that knows how to access items from an underlying collection
(en.getNext()/en.hasMoreElements() in our case) one at a time, whilst
keeping track of its current position within that sequence.

A highly simplistic implementation of this stolen from MDN:

> function makeIterator(array) {
>     var nextIndex = 0;
>     
>     return {
>        next: function() {
>            return nextIndex < array.length ?
>                {value: array[nextIndex++], done: false} :
>                {done: true};
>        }
>     };
> }

When yield is called, the remainder of the generator function will
continue to run on the _subsequent_ invocation of next().  The next()
function returns an object literal that looks like

	{value: <value>, done: <boolean>}

where the yielded variable’s value is stored and where the done
boolean indicates if we’ve reached the end of the iterator.  This is
what the for…of loop uses to assign values and detect when to stop
iterating.

Let me construct a fictions example:

> let data = [1,2,3,4,5];
> 
> function* iter () {
>   for (let i = 0; i < data.length; i++) {
>     console.log("before yield");
>     yield data[i];
>     console.log("after yield");
>   }
> }
> 
> let gen = iter();
> 
> console.log(gen.next().value);
> console.log(gen.next().value);
> console.log(gen.next().value);

The above example produces the following output:

> before yield
> 1
> after yield
> before yield
> 2
> after yield
> before yield
> 3

You will note that "after yield" is not logged because there is no
fourth call to gen.next().

If you place a break after the yield statement, you return from the
index iteration over data, also causing the iter function to return and
the iterator object to loose its underlying data collection:

> before yield
> 1
> undefined
> undefined

The second and third next() invocation will return undefined because
the index loop is no longer running and no longer assigning data to the
object literal’s value field.

I hope this helps, but you can read more about generators in
https://developer.mozilla.org/en/docs/Web/JavaScript/Guide/Iterators_and_Generators
or at https://en.wikipedia.org/wiki/Generator_(computer_programming).

> You can vary `host` and `hostname` like I do above in my example for the generator code. Just adding some variations should be fine, right?

Let’s be clear: This had _no_ tests before this patch, so this
is arguably an improvement.  The primary reason I don’t want
to write more detailed tests for this is because I know the
implementation is buggy and wrong.  I intend to add more tests
when it has been made standards conforming, which is scheduled for
https://bugzilla.mozilla.org/show_bug.cgi?id=1372582.  I don’t see the
point in writing tests that I need to rewrite in the future.
Comment on attachment 8876786 [details]
Bug 1371733 - Move cookie service to chrome space;

https://reviewboard.mozilla.org/r/148110/#review156746

> Err, the spec says we have to return an `InvalidArgumentError`. So why not using `assert.string()` in all those cases when the key exists?

When deserialising the cookie from JSON in cookie.fromJSON, we do return
an InvalidArgumentError so in theory this should never happen unless
cookie.add is called with an malformed cookie object.  Arguably the
check should maybe be done in testing/marionette/driver.js as it is a
WebDriver specialisation.

The point I made earlier about separation of backend services and
WebDriver characteristics applies here, but I don’t have very strong
opinions on whether it is semantically more correct to return a
TypeError here.  I guess I don’t care enough to debate this nuance,
however.
Comment on attachment 8876786 [details]
Bug 1371733 - Move cookie service to chrome space;

https://reviewboard.mozilla.org/r/148110/#review155536

> My earlier explanation that “it returns immediately” did not mean to
> imply that it is analogous to returning a value.  I’m sorry if this
> was misleading.
> 
> A generator function, identified by "*" and a yield statement,
> provides a so called ‘iterator object’ or [Symbol.Iterator]
> that knows how to access items from an underlying collection
> (en.getNext()/en.hasMoreElements() in our case) one at a time, whilst
> keeping track of its current position within that sequence.
> 
> A highly simplistic implementation of this stolen from MDN:
> 
> > function makeIterator(array) {
> >     var nextIndex = 0;
> >     
> >     return {
> >        next: function() {
> >            return nextIndex < array.length ?
> >                {value: array[nextIndex++], done: false} :
> >                {done: true};
> >        }
> >     };
> > }
> 
> When yield is called, the remainder of the generator function will
> continue to run on the _subsequent_ invocation of next().  The next()
> function returns an object literal that looks like
> 
> 	{value: <value>, done: <boolean>}
> 
> where the yielded variable’s value is stored and where the done
> boolean indicates if we’ve reached the end of the iterator.  This is
> what the for…of loop uses to assign values and detect when to stop
> iterating.
> 
> Let me construct a fictions example:
> 
> > let data = [1,2,3,4,5];
> > 
> > function* iter () {
> >   for (let i = 0; i < data.length; i++) {
> >     console.log("before yield");
> >     yield data[i];
> >     console.log("after yield");
> >   }
> > }
> > 
> > let gen = iter();
> > 
> > console.log(gen.next().value);
> > console.log(gen.next().value);
> > console.log(gen.next().value);
> 
> The above example produces the following output:
> 
> > before yield
> > 1
> > after yield
> > before yield
> > 2
> > after yield
> > before yield
> > 3
> 
> You will note that "after yield" is not logged because there is no
> fourth call to gen.next().
> 
> If you place a break after the yield statement, you return from the
> index iteration over data, also causing the iter function to return and
> the iterator object to loose its underlying data collection:
> 
> > before yield
> > 1
> > undefined
> > undefined
> 
> The second and third next() invocation will return undefined because
> the index loop is no longer running and no longer assigning data to the
> object literal’s value field.
> 
> I hope this helps, but you can read more about generators in
> https://developer.mozilla.org/en/docs/Web/JavaScript/Guide/Iterators_and_Generators
> or at https://en.wikipedia.org/wiki/Generator_(computer_programming).

Ok. Thanks. The last part was pretty helpful.

> Let’s be clear: This had _no_ tests before this patch, so this
> is arguably an improvement.  The primary reason I don’t want
> to write more detailed tests for this is because I know the
> implementation is buggy and wrong.  I intend to add more tests
> when it has been made standards conforming, which is scheduled for
> https://bugzilla.mozilla.org/show_bug.cgi?id=1372582.  I don’t see the
> point in writing tests that I need to rewrite in the future.

I will take you by your word. :)
Comment on attachment 8876786 [details]
Bug 1371733 - Move cookie service to chrome space;

https://reviewboard.mozilla.org/r/148110/#review157124
Attachment #8876786 - Flags: review?(hskupin) → review+
Pushed by atolfsen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/abaefde1377f
Encode currentURL as URL object; r=whimboo
https://hg.mozilla.org/integration/autoland/rev/5f5621f58449
Singularise cookie module name; r=whimboo
https://hg.mozilla.org/integration/autoland/rev/4a22db2d525f
Propagate error message of assert.object; r=whimboo
https://hg.mozilla.org/integration/autoland/rev/fef1ad177d24
Add tests for custom assertion errors; r=whimboo
https://hg.mozilla.org/integration/autoland/rev/24650e8a712b
Move cookie service to chrome space; r=whimboo
Product: Testing → Remote Protocol
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: