Skip to content
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

Redesign URLUtils #62

Closed
annevk opened this issue Aug 15, 2015 · 8 comments
Closed

Redesign URLUtils #62

annevk opened this issue Aug 15, 2015 · 8 comments

Comments

@annevk
Copy link
Member

annevk commented Aug 15, 2015

Otherwise they do not operate with the latest base URL in mind.

At which point we might want to consider a slightly different design for these that does not involve an internal url. 😟

@annevk
Copy link
Member Author

annevk commented Aug 18, 2015

Location, WorkerLocation (does not have setters)

  • getters: return component of internal URL
  • setters: parse given value; if valid, navigate to copy of internal URL modified by parsed value

<a>, <area>

  • getters: parse internal input against base URL to create new internal URL; return component from internal URL
  • setters: parse internal input against base URL to create new internal URL; parse given value; if valid, change internal input to serialized internal URL modified by parsed value

URL

  • getters: return component of internal URL
  • setters: run setter to change internal URL

Note that because we need to determine whether the value passed to the setter is valid, they cannot be part of the URL parser most likely. Although I should investigate returning failure more and see if that's sufficient.

Also, for <a>, <area>, once you set something you no longer need to be concerned with the base URL changing. Most likely with the exception of then setting href somehow.

@annevk annevk changed the title URLUtils attribute setters also need to reset the input Redesign URLUtils Aug 18, 2015
@annevk
Copy link
Member Author

annevk commented Aug 18, 2015

So what does this mean?

  • base URL is important for everything but WorkerLocation (due to lack of setters)
  • base URL change is important for everything but WorkerLocation and URL (due to lack of changing base URL)
  • base URL change affects Location differently from <a> and <area>. For Location only href is affected.
  • <a> and <area> effectively don't need an internal URL since unless you cache the base URL in your implementation (which we should suggest in a note) it will be invalidated for each access.
  • The URL parser when an override is given needs to return "failure" whenever the input did not lead to a change. For setting scheme we need to return silent-failure as well (for input that was "valid" but rejected, see URLUtils is wrong for Location object #61). However that does mean it needs to return success too. Probably in step 9 for path/query/fragment and at various other places.

Another thing that seems mutually exclusive is that <a> and <area> need input, whereas the others need url. We should maybe use that so the various members can branch on that although it might not be as clear as things could be...

@domenic
Copy link
Member

domenic commented Aug 18, 2015

Thoughts:

  • <a> and <area> plan sounds good, although I'd replace "internal URL" with "temporary URL" since it doesn't need to be stored in any sort of internal slot. Also "internal input" is a bit confusing when you really just mean "the value of the href attribute".
  • URL sounds straightforward and good.
  • Still not entirely clear on Location:
    • For getters, why do you need an internal URL? Can't you just use the document's URL?
    • Hash setter may be special-cased, but maybe it just falls out of synchronous navigation updating the document's URL, which as long as you straighten out my first point will work fine.
  • "base URL change is important for everything" I would not include the base URL change concept at all. Instead you just lazily get the new base URL in the appropriate getter. No need to "push" updates. This will be a drastic simplification to the spec and also apparently matches browsers better.

@annevk
Copy link
Member Author

annevk commented Aug 19, 2015

The only reason we'd want an "internal URL" for <a> and <area> is blob URLs I think. Although that does seem like significant complexity for a somewhat broken feature. Ugh.

@annevk
Copy link
Member Author

annevk commented Aug 20, 2015

So <a> and <area> could supply "retrieve the input" and "forward the input" functions that URLUtils would use to effectively get/set the href attribute value. Additionally they would supply an "internal URL" to make blob URLs work.

WorkerLocation supplies a pointer to an "internal URL".

Location supplies a pointer to an "internal URL" and a "forward the URL" function. (If this function is supplied, the "internal URL" is not updated.)

URL supplies an "internal URL".

With these four concepts I should be able to define this in a way that works for all these objects. Additionally some setters that need to throw might have additional special casing.

@annevk
Copy link
Member Author

annevk commented Aug 25, 2015

Here is another sketch of the various behaviors we have. <a>/<area> are the only ones that are also backed by a string (they need the url object backing for blob URLs) and the only ones that require a base URL reset.

reset
  1. if non-relative flag is set, terminate.
  2. let parsedURL = parse URL using get the input / get the base
  3. set url to parsedURL

<a>/<area>

  .href getter

    1. reset
    2. if url is non-null, return url serialized
    3. return input

  .href setter

    1. let input be given value
    2. let parsedURL = parse URL using input / get the base
    3. set url to parsedURL
    4. set input to input

  other getter

    1. reset
    2. if url is non-null, return url's other, serialized
    3. return the empty string

  other setter

    1. reset
    2. parse the given value using url. (This modifies url.)
    3. set input to url, serialized

Location

  .href getter

    1. Return url, serialized.

  .href setter

    1. let parsedURL = given value / get the base
    2. if parsedURL is failure, throw
    3. forward parsedURL

  other getter

    1. Return url's other, serialized.

  other setter

    1. Let copyURL be a copy of url.
    2. Let result be parse the given value using copyURL. (This modifies copyURL.)
    3. If result is failure, throw / terminate these steps (depends on other)
    4. forward copyURL

WorkerLocation

  See Location, but minus the setters.

URL

  .href getter

    1. Return url, serialized

  .href setter

    1. let parsedURL = given value / get the base
    2  if parsedURL is failure, throw
    3. set url to parsedURL

  other getter

    1. Return url's other, serialized

  other setter

    1. parse the given value uusing url. (This modifies url.)

Location is the most special due to its behavior for setters. Perhaps the best approach is just to specify its setters separately.

@domenic
Copy link
Member

domenic commented Aug 25, 2015

reset

I'd suggest minimizing side effects for clarity. So instead of reset actually setting url to parsedURL, have reset return parsedURL, and have each algorithm step do "set url = reset()". (Hmm, maybe you can get rid of reset entirely? I don't understand what it's consulting when you say "if non-relative flag is set"... do <a>/<area> have non-relative flags?)

parse the given value using url. (This modifies url.)

As written this is pretty unclear. But I trust you'll have a way of writing it more clearly when you formalize it. Again I would urge avoiding side effects, so you can do something more like "set url = parse(given value, url)".

.href setter

Make sure to add a note explaining that it explicitly does not update anything, and that the result is that location.href.pathname = "foo"; location.href.pathname does not return "foo"

Let result be parse the given value using copyURL. (This modifies copyURL.)

Same issue as before about avoiding side effects.


Let's explicitly call out the internal state and provided hooks in each case:

  • <a>/<area>
    • Internal slot: url (maybe non-relative flag? confused)
    • Provided hooks: get the base; get the input
  • Location/WorkerLocation
    • Internal slot: url
    • Provided hooks: get the base; forward
  • URL
    • Internal slots: url; base url
    • Provided hooks: get the base (returns base url slot value)

@annevk
Copy link
Member Author

annevk commented Aug 25, 2015

<a>/<area> also need "set the input".

annevk added a commit to whatwg/html that referenced this issue Sep 23, 2015
annevk added a commit to whatwg/html that referenced this issue Sep 24, 2015
annevk added a commit to whatwg/html that referenced this issue Sep 25, 2015
The URLUtils abstraction is not working out. See #164 and whatwg/url#62 for details.
annevk added a commit to whatwg/html that referenced this issue Oct 1, 2015
See whatwg/url#62 for the background. This
defines a mixin used by both a and area elements to define their IDL
members previously defined by the URL standard.

It also fixes a small error in the WorkerLocation text.
annevk added a commit to whatwg/html that referenced this issue Oct 1, 2015
See whatwg/url#62 for the background. This
defines a mixin used by both a and area elements to define their IDL
members previously defined by the URL standard.

It also fixes a small error in the WorkerLocation text.
annevk added a commit to whatwg/html that referenced this issue Oct 1, 2015
This fixes the HTML standard side of these issues:

* whatwg/url#61
* whatwg/url#62

There is still some cleanup work left on the URL side, e.g., making
sure the basic URL parser returns failure for certain conditions when
override state is scheme start state.
annevk added a commit to whatwg/html that referenced this issue Oct 2, 2015
This fixes the HTML standard side of these issues:

* whatwg/url#61
* whatwg/url#62

There is still some cleanup work left on the URL side, e.g., making
sure the basic URL parser returns failure for certain conditions when
override state is scheme start state.
annevk added a commit to whatwg/html that referenced this issue Oct 6, 2015
See whatwg/url#62 for the background. This
defines a mixin used by both a and area elements to define their IDL
members previously defined by the URL standard.

It also fixes a small error in the WorkerLocation text.
annevk added a commit to whatwg/html that referenced this issue Oct 6, 2015
See whatwg/url#62 for the background. This
defines a mixin used by both a and area elements to define their IDL
members previously defined by the URL standard.

It also fixes a small error in the WorkerLocation text.
annevk added a commit to whatwg/html that referenced this issue Oct 6, 2015
This fixes the HTML standard side of these issues:

* whatwg/url#61
* whatwg/url#62

There is still some cleanup work left on the URL side, e.g., making
sure the basic URL parser returns failure for certain conditions when
override state is scheme start state.
annevk added a commit to whatwg/html that referenced this issue Oct 6, 2015
See whatwg/url#62 for the background. This
defines a mixin used by both a and area elements to define their IDL
members previously defined by the URL standard.

It also fixes a small error in the WorkerLocation text.
annevk added a commit to whatwg/html that referenced this issue Oct 6, 2015
This fixes the HTML standard side of these issues:

* whatwg/url#61
* whatwg/url#62

There is still some cleanup work left on the URL side, e.g., making
sure the basic URL parser returns failure for certain conditions when
override state is scheme start state.
@annevk annevk closed this as completed in 4cb6328 Oct 12, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

3 participants
@domenic @annevk and others