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

add saveData attribute to NetworkInformation #66

Merged
merged 3 commits into from Oct 2, 2017
Merged

Conversation

igrigorik
Copy link
Member

@igrigorik igrigorik commented Aug 25, 2017

Attribute reflects value advertised by Save-Data header defined by Clients Hints specification.

Open questions to address:

  • Should saveData attribute be mapped 1:1 to the value advertised by the Save-Data HTTP header?
    • Keeping them in sync is nice, but on the other hand, may not map to the nicest JS API.. Right now you'd have to do "on" string comparisons.
    • Currently, Save-Data only registers a single token ("on"), but we wanted to reserve the option of extending this in the future, so reducing this to a boolean value is probably unwise.. Then again, if we don't map header value and JS APIs directly, we could expose different attributes in JS-land for each header value.
  • Should changes in saveData value fire onchange event?

As an aside, it may also make sense to migrate Save-Data definition into this spec, instead of defining it in CH; that way we can spell out the processing and interop between them more cleanly.

Closes #42.

/cc @tarunban @jeffposnick @marcoscaceres @bengreenstein


Preview | Diff

Attribute reflects value advertised by Save-Data header defined by
Clients Hints specification [1].

[1]
https://tools.ietf.org/html/draft-ietf-httpbis-client-hints-04#section-3.5
@bengreenstein
Copy link

While I think "if (saveData === 'on')" is a little less elegant than "if (saveData) {...}, " I don't want to paint ourselves into a corner. I can easily imagine adding the value 'extreme' in the future.

I think a change in the saveData value should fire an onchange event, as some people do toggle the Data Saver setting in Chrome a fair bit.

@igrigorik
Copy link
Member Author

While I think "if (saveData === 'on')" is a little less elegant than "if (saveData) {...}, " I don't want to paint ourselves into a corner. I can easily imagine adding the value 'extreme' in the future.

Technically, it doesn't have to. We can easily expose different JS-land attributes for each enum value, instead of asking developers to compare strings. Also, stepping back, do you envision more values in the header, and what would they be?

I think a change in the saveData value should fire an onchange event, as some people do toggle the Data Saver setting in Chrome a fair bit.

sgtm.

@bengreenstein
Copy link

So you're saying do something like "if (saveData) { if (saveDataExtreme) { ... }}" ? That seems fine to me.

@bengreenstein
Copy link

The only additional value we've considered is something along the lines of "extreme" to indicate that data is extremely expensive (e.g., when roaming) and so the absolute minimal page should be served. This is in contrast to "on," which indicates that the user is still interested in an almost full fidelity page, but would appreciate data reductions that don't interfere too much with the experience.

@igrigorik
Copy link
Member Author

Interesting. What's the current thinking for where and how we'll obtain the "extreme" signal? User setting, platform / OS signals, other means? I see a few options for how we can interpret them:

  1. We can keep ~saveData and ~expensiveData as separate attributes.
    • Q: would we ever have the case where expensiveData=true but saveData=false? E.g. what if I'm roaming but really want the super high-fidelity photo download?
  2. Or, is ~expensiveData=true implicitly means that saveData=true?
    • If that's the case, then perhaps an enum does make more sense.

@bengreenstein
Copy link

I was thinking that the browser would convey how much data savings is desired, not how expensive the data is. So if saveAsMuchDataAsPossible is true, saveData should also be true. I also think we should not all saveAsMuchDataAsPossible=true, saveData=false, because many sites will only have one data saving implementation. If that implementation isn't extreme, the site could just check saveData. If it is extreme, it could check saveAsMuchDataAsPossible.

We could instead attempt to surface how expensive the data is, but I'm afraid the best we can do now is to use heuristics or classified a user's historical data use, which would be extremely coarse.

@igrigorik
Copy link
Member Author

Hmm ok. Sounds like we want saveData as a boolean then, not an enum, plus an additional property communicate the "severity"...? In theory, doesn't effectiveType hint at this already?

if (navigator.connection.saveData) {
  if (navigator.connection.effectiveType == "slow-2g") { 
     // super duper compression 
  } else { 
    // ...
  } 
}

Translating above to headers, we might have:

Save-Data: on;slow-2g

Given that we already expose ECT, do we need any additional signals?

@bengreenstein
Copy link

An effectiveType slower than 3g indicates a need for super duper compression, as does an actual connection type of 2g, but there are likely many other signals that could contribute to a need for it, e.g., a user setting. All that said, I think a boolean is fine for now and we can invent an additional property to communicate severity if we eventually need it.

- Switching to boolean, based on discussion in
#66
- Added saveData to "handle changes to underlying connection" algorithm
  to trigger onchange if+when saveData pref is updated
Moving definition from CH into NetInfo, which allows us to connect JS
API to Fetch processing, and keeps things in one place.
@igrigorik
Copy link
Member Author

@bengreenstein updated, ptal.
@mnot per discussion earlier, also moving Save-Data definition into NetInfo -- ptal.

Copy link

@mnot mnot left a comment

Choose a reason for hiding this comment

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

lgtm

@igrigorik
Copy link
Member Author

igrigorik commented Sep 12, 2017

Thanks Mark! @bengreenstein @tarunban any feedback on your end?

@bengreenstein
Copy link

lgtm

@igrigorik igrigorik changed the title [WIP] add saveData attribute to NetworkInformation add saveData attribute to NetworkInformation Oct 2, 2017
sd-token = token
</pre>

This specification defines the "`on`" `sd-token` value, which is used as a signal indicating explicit user opt-in into a reduced data usage mode (i.e. when <a>saveData</a>'s value is `true`), and when communicated to origins allows them to deliver alternate content honoring such preference - e.g. smaller image and video resources, alternate markup, and so on.
Copy link
Collaborator

Choose a reason for hiding this comment

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

'on`: Should not the attribute value be true/false (same as in the request header)?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, these are distinct. "on" value in the header is mapped to saveData == true, and all other values to "false".

@igrigorik
Copy link
Member Author

Thanks everyone for reviews and feedback, merging.

Copy link
Collaborator

@tarunban tarunban left a comment

Choose a reason for hiding this comment

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

Comment below.

@igrigorik igrigorik merged commit 10fdd2d into gh-pages Oct 2, 2017
Copy link
Collaborator

@tarunban tarunban left a comment

Choose a reason for hiding this comment

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

lgtm

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants