harvesthq / platform

A whole new way to add time tracking to your web application.
https://www.getharvest.com/platform
59 stars 11 forks source link

Intelligently use http or https #2

Closed tjschuck closed 11 years ago

tjschuck commented 11 years ago

We shouldn't pull this over HTTPS if it's not necessary. It'll be a lot slower:

$ ab -n 100 http://platform.harvestapp.com/assets/platform.js

Time taken for tests:   23.060 seconds
Requests per second:    4.34 [#/sec] (mean)
Time per request:       230.596 [ms] (mean)

Connection Times (ms)
              min  mean[+/-sd] median   max
Connect:       74   75   1.4     74      83
Processing:   150  156  42.2    150     572
Waiting:       75   76   2.6     75      97
Total:        224  231  42.4    225     648

$ ab -n 100 https://platform.harvestapp.com/assets/platform.js

Time taken for tests:   39.241 seconds
Requests per second:    2.55 [#/sec] (mean)
Time per request:       392.411 [ms] (mean)

Connection Times (ms)
              min  mean[+/-sd] median   max
Connect:      236  241   4.0    239     259
Processing:   150  152   2.3    151     168
Waiting:       75   77   2.2     76      94
Total:        387  392   4.9    391     411

This new code uses the protocol of the including page.

We should probably do the same thing with the stylesheet that gets included (and anything else that I'm not thinking of). But that's application-side stuff -- I'll do this easy one and leave that as an exercise to the reader.

tjschuck commented 11 years ago

/cc @rbright @evanwalsh and maybe @warwickp to quickly confirm that this is kosher.

evanwalsh commented 11 years ago

Shouldn't we just change the URL to start with // and call it a day?

jkintscher commented 11 years ago

Yea, just make it a relative URL without a scheme. It's valid (as per some RFC standard) and very well supported.

rbright commented 11 years ago

It's all relative with the extension now.

tjschuck commented 11 years ago

I'm normally totally cool with protocol-relative URLs, but since this is going to be "out in the wild" and included on other sites outside of our control, it might be safer to specify that it only uses HTTP or HTTPS.

Two points, one slightly ridiculous but potentially valid, and the other technically worth looking into:

  1. Using the code in this pull req makes it only use HTTP(S). What about Gopher, man? Alright, that's ridiculous, but what about some kind of proprietary in-house usage? Some corporate intranet app that uses some kind of silly protocol? An app specific scheme in some native thing using a lot of web views? Someone making some kind of Outlook-embeded macro thing like a crazy person, where // will end up being a Windows file-system reference? I know this is bordering on silly, but hey, just exploring possibilities here.
  2. Will shitty browsers choke? And/or download the file twice? It looks like double-download is only a problem with stylesheets, although this whole protocol thing also applies to the stylesheet that's being downloaded by the platform, and that should be fixed too. (Also, I just saw that I commented on that article 2.5 years ago. Weird.)

TL;DR: protocol-relative is more elegant, but explicitly pushing HTTPS with an explicit HTTP fallback seems safer.

But all of this is kind of edge-casey, and I wouldn't be upset in the least if you think I'm just being a crazy person.

rbright commented 11 years ago

Including the script via HTTP will yield "Insecure Content" warnings and, in some cases, not load the content until the user explicitly chooses to do so. I have no qualms with choosing one or the other, but my vote is for HTTPS. It may be slower, but it also avoids the afformentioned user experience problem.

tjschuck commented 11 years ago

@rbright Wait, what? I don't think we're talking about the same thing. Clarify yo'self, boo.

rbright commented 11 years ago

If I include platform.js over HTTP on a site using HTTPS, it yields an "Insecure Content" warning. In Chrome, this means that the script is not loaded until the user explicitly does so through the icon in Chrome's Omnibox.

tjschuck commented 11 years ago

Right, I know -- this isn't about using HTTP instead of HTTPS, it's about using HTTP when HTTPS isn't necessary:

s.src = ('https:' == document.location.protocol ? 'https' : 'http') + '://platform.harvestapp.com/assets/platform.js';

If the page is HTTPS, use HTTPS. Otherwise, use HTTP.

rbright commented 11 years ago

Gotcha. In that case, we can just use something like what you have above.

zmoazeni commented 11 years ago

On point 1, could we just deploy and fix it if it does come up? I'm very pro-relative protocol. Saying being explicit is safer is like saying manual memory management is safer. It's true, but it's also long enough rope to hang yourself.

I may not have the full context though. But I'd suggest trying the best solution until it breaks.

zmoazeni commented 11 years ago

Related: http://blog.wikimedia.org/2011/09/27/protocol-relative-urls-enabled-on-all-wikimedia-foundation-wikis/ it would be interesting to figure where this ended up (over a year old). I would hate to have to manage Wikipedia's browser support.

rbright commented 11 years ago

@zmoazeni There are a few dependencies dynamically loaded by the platform that would be easily managed by deploying a few modifications to platform.js. Additionally, we'd wish to include platform.js on a target page using the same method, so we'd need to update the Chrome extension.

According to the double download article @tjschuck posted, it seems that IE 7/8 will be impacted. If we plan to support IE 8+, that might factor into the decision here.