[GuillaumeGomez]

Fixes #61239.

@starblue: Can you give a try to this change please? I tried on chrome and firefox and both worked so if you're using another web browser, that might be useful. :)

r? @Manishearth

[starblue]

I'm using Firefox 67.0.
I tried to build Rust, but stage 1 compilation of compiler artifacts failed, I can see that it was killed.
(I suppose that was by the OOM killer due to lack of memory, as the machine only has 8GB of which about 4GB are free. I've had similar problems before with a large svd2rust-generated crate.)
So I'll wait until the fix is in nightly and check that.

[GuillaumeGomez]

Hum, that's not very ideal. This fix is pretty simple but it'd have been nice to have a confirmation before actually merging it in case it was useless or not solving your issue.

[starblue]

I finally managed to build it with -j 1, and I can report that the issue is indeed fixed. Thanks!

[GuillaumeGomez]

Perfect, thanks! Just waiting for someone to review now.

cc @rust-lang/rustdoc

[ollie27]

What is the typeof window !== 'undefined' check for?

[GuillaumeGomez]

Good catch! I'll update the code soon.

[ollie27]

It looks like this is actually quite tricky to get right. How about borrowing the code from https://developer.mozilla.org/en-US/docs/Web/API/Web_Storage_API/Using_the_Web_Storage_API#Testing_for_availability instead?

[GuillaumeGomez]

This code seems quite long to run (to be confirmed!). Saving its state should be enough though.

[ishitatsuyuki]

Pro tip: when letting others test a build, you can @bors try then ask them to use https://github.com/kennytm/rustup-toolchain-install-master.

[GuillaumeGomez]

@ollie27: The code sample you linked checks too many things for us (like if the localStorage is full, which we don't really care in our case considering how little we use it). Checking if the localStorage exists is enough normally so I'll just fix the error you noticed.

[GuillaumeGomez]

@ishitatsuyuki Thanks for the tip!

[GuillaumeGomez]

Updated.

[QuietMisdreavus]

@ollie27: The code sample you linked checks too many things for us (like if the localStorage is full, which we don't really care in our case considering how little we use it). Checking if the localStorage exists is enough normally so I'll just fix the error you noticed.

According to the MDN page they linked, we may still get errors about localStorage being full, even with our relatively light usage of it:

One example of that is Safari, which in Private Browsing mode gives us an empty localStorage object with a quota of zero, effectively making it unusable.

Also, couldn't we move the final return line into the try block? It could even replace the current contents, since we're still accessing window.localStorage which would throw the exception.

[GuillaumeGomez]

Interesting. I'll check on safari to see its behaviour.

[GuillaumeGomez]

Also, couldn't we move the final return line into the try block? It could even replace the current contents, since we're still accessing window.localStorage which would throw the exception.

Excellent idea! I did it.

According to the MDN page they linked, we may still get errors about localStorage being full, even with our relatively light usage of it:
...

I tried on safari, both private and "normal" modes worked fine (I could set settings everything just fine).

[Dylan-DPC-zz]

ping from triage @Manishearth waiting for your review on this

[Manishearth]

@bors r+

(note for triage: i'm helping out with rustdoc reviews, but please don't block rustdoc reviews on me)

[GuillaumeGomez]

Nothing happened apparently? I'll close then reopen and give it another try.

[GuillaumeGomez]

@bors: r=Manishearth

[bors]

📌 Commit 4eb19d1 has been approved by Manishearth

[bors]

⌛ Testing commit 4eb19d1 with merge 71f9384...

[bors]

☀️ Test successful - checks-azure, checks-travis, status-appveyor
Approved by: Manishearth
Pushing 71f9384 to master...