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

Update user.js #583

Merged
merged 4 commits into from
Dec 10, 2018
Merged

Update user.js #583

merged 4 commits into from
Dec 10, 2018

Conversation

earthlng
Copy link
Contributor

things not in the PR:

  • 0911 - remove? or replace with network.auth.subresource-http-auth-allow (would need to add it to troubleshooter as well)
  • 1022 - does it even do what we expect it to do? remove?
  • 1700 - SETUP-CHROME and maybe add a recommendation for TC?
  • 2002 - add test page?
  • 2302 - [FF32+] - added in 32, enabled in 44 (still disabled in ESR60!)

@earthlng
Copy link
Contributor Author

just a couple things I'd like to change ...

👖, 🐈

@Thorin-Oakenpants
Copy link
Contributor

Thorin-Oakenpants commented Dec 10, 2018

see #575 OP .. 1022 = inactive or remove (I'm fine with removing it)

  • make 1022 resume from cash inactive or remove it
    • even if you wipe history on close, a crash is not a graceful exit, so the file remains, and 99% of people would want to restore from a crash, surely

@earthlng
Copy link
Contributor Author

the only reason why it could make sense to disable it IMO (assuming it even does disable session restore after crash!) would be if a tab manages to take down the whole browser you probably don't want to restore that tab

@Thorin-Oakenpants
Copy link
Contributor

1260: that's web breakage - if sha-1 is restricted, pages won't load .. web breaks. Just like not allowing DRM breaks video, video will not load. I know we can argue semantics here.

user_pref("devtools.webide.adaptersAddonURL", ""); - why add a deprecated in FF57 pref? I don't mind, just wondering why. And is that in the [-] reference link?

The rest looks fine on first glance. I still need to go thru [SETUP* and make sure we have things in the right places. end of 1st line if ok, but in body if the next comment specifically is about that. So yeah, those moves look ok as well

@earthlng
Copy link
Contributor Author

1260 - no it's not. The default is to block sha1 and only allow them for local added roots ie redirecting traffic through fiddler, an Antivirus, etc. Our default basically just prevents traffic from being routed like that if the intermediary app is using a sha1 cert. So it's more a CHROME thing than WEB, IMHO

@Thorin-Oakenpants
Copy link
Contributor

the only reason why it could make sense to disable it IMO (assuming it even does disable session restore after crash!) would be if a tab manages to take down the whole browser you probably don't want to restore that tab

So keep it, but make inactive? we can drop the [setup if inactive

/* 1022: disable resuming session from crash
 * Useful if repeated crashes on restore are caused by a malicious tab ***/
 // user_pref("browser.sessionstore.resume_from_crash", false);

@Thorin-Oakenpants
Copy link
Contributor

OK, answer me this. If 1260 causes a problem, what does the end user experience? Is it a redirected page with a message?

@earthlng
Copy link
Contributor Author

why add a deprecated in FF57 pref?

because it came up in some of those 400+ pref dumps and I think TB also still has that pref active.
And yes, it was removed in the same [-] that we already link to

@Thorin-Oakenpants
Copy link
Contributor

because it came up in some of those 400+ pref dumps

thats fine. Will need to add it to the deprecated sticky

@earthlng
Copy link
Contributor Author

Useful if repeated crashes on restore are caused by a malicious tab

that implies that it does what we think it does. You're the one with the scratchpad to crash FF ... :)

@Thorin-Oakenpants
Copy link
Contributor

1700 - SETUP-CHROME and maybe add a recommendation for TC

No need to add setup tag (and I am going to get those all out of headers). Recommendation for TC, IDK - it hasn't really been updated for like 6 months from memory - mostly lint and things. The issues are piling up.

@Thorin-Oakenpants
Copy link
Contributor

Can I do a little editing to undo a couple of things and push this thru. I'd like to address the others one by one - i think my comment about not wanting make a note/todo list before was ambiguous. I meant for a single change it wasn't worth opening an issue.

@earthlng
Copy link
Contributor Author

If 1260 causes a problem, what does the end user experience?

AFAIK that pref alone does nothing if you don't also set a proxy, fe fiddler. So most people will never have a problem with that pref and people who use fiddler or similar should have no problem knowing where to look to make it work. (1200 = HTTPS, CERTS, etc)
IMO we can just drop that tag all together so we don't have to argue WEB vs CHROME :)

@earthlng
Copy link
Contributor Author

Can I do a little editing to undo a couple of things and push this thru

sure

@earthlng
Copy link
Contributor Author

re: 1700 - "No need to add setup tag"

I mean currently due to needing to long-press to even have the option to use containers show up on +, some (most?) people won't even notice that we enabled containers. That's why I think it needs a setup tag of some kind.

@earthlng
Copy link
Contributor Author

I'll update the deprecated sticky

@Thorin-Oakenpants
Copy link
Contributor

I mean currently due to needing to long-press to even have the option to use containers show up on +, some (most?) people won't even notice that we enabled containers. That's why I think it needs a setup tag of some kind.

Oh, i get ya. We can add one to the long press pref if you want. As for noticing it's on - users might notice them in right click contexts, and it will show in options as well. That should suffice

@earthlng
Copy link
Contributor Author

maybe change
1704: set long press behaviour on "+ Tab" button to display container menu
to
1704: set behaviour on "+ Tab" button to display container menu
and 2=the menu is shown on long press ?

@earthlng
Copy link
Contributor Author

earthlng commented Dec 10, 2018

0=no menu (default), 1=show when clicked, 2=show on long press
and
[NOTE] The menu does not contain a non-container tab option (use Ctrl+T to open non-container tab)

edit: actually I think we can just remove the NOTE. Option 2 allows to use non-container and containerized tabs. 1 more reason to just add the TAG to the section title IMHO.

@Thorin-Oakenpants Thorin-Oakenpants merged commit 71a2d39 into master Dec 10, 2018
@Thorin-Oakenpants Thorin-Oakenpants deleted the earthlng-patch-1 branch December 10, 2018 22:23
@earthlng
Copy link
Contributor Author

^^ see edit:

actually I think we can just remove the NOTE. Option 2 allows to use non-container and containerized tabs. 1 more reason to just add the TAG to the section title IMHO.

@claustromaniac
Copy link
Contributor

  • 0911 - remove? or replace with... 🐈: replace! (with user_pref("network.auth.subresource-http-auth-allow", 0); or 1? I'd go with 0, but IDK I'M NEW AT THIS SHIT!!! 🙀 *freaking out*).

Recommendation for TC, IDK - it hasn't really been updated for like 6 months from memory - mostly lint and things. The issues are piling up.

who cares about a bunch of minor issues and feature requests not getting solved as long as the shit works? especially when we talk about such a useful extension! My own shitty extensions are heading the same way, except they're not nearly as useful!!! 😅 It is already recommended on the wiki though, so it doesn't matter anyway.

@Thorin-Oakenpants
Copy link
Contributor

[NOTE] kinda lends weight (while our active value is the best so as to not spring nasty surprises - if FF ever flip containers on, I bet they go for 2 as well). I like that we added a setup tag to draw attention to it (the setting as well as the fact we turned containers on). The other prefs make no difference.

And, I want to get [setup tags against prefs. They don;t work with the real-time thingy if they are stuck in section headers. That doesn't mean we can't also have them in headers, but it creates duplicity. At the end of the day, I want users in the js and on the whizzy-do dactyl site, to be able to jump from pref to pref and make changes or troubleshoot. The less in section headers the better

@Thorin-Oakenpants
Copy link
Contributor

OK, I need to DL a copy and compare with my local master, then I'll start a new issue, or commandeer one, and add in all the things we haven't addressed from here

@Thorin-Oakenpants
Copy link
Contributor

who cares about a bunch of minor issues and feature requests

Yeah I'm fine with it being recommended. Was just querying its status (I don't use it). From the outside looking in, I was just querying it's status. For all I know, it's stable AF. I'll defer to you and 🌎 since you gals use it. It's on the task list FYI

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

Successfully merging this pull request may close these issues.

None yet

3 participants