Haketilo

Any conventions I haven't commented on I am perfectly satisfied with and cannot further elaborate.

Function definitions ... This is inspired from Linux coding convention (which, I know, is directed at a different language).

That's where it's from! I'd never seen it before. That said, there is no problem with it to me; the only thing missing is an explicit standard for loops/conditionals without braces.

I stuck to only using block comments, in the way Linux does it. I came to like it personally. It might not be worth to impose this on others, though.

Personally, I think using block and inline-style comments interchangeably is fine (except for multiline comments; treating them as a series of single-line comments is just distasteful). A more useful restriction in my opinion would be to avoid putting mixing code and comments on a line (which I solemnly swear not to do in future).

Indentation is ... This really sucks and I stuck to this simply because I was too lazy to change the defaults.

This works fine for me! However, as an additional layer of assurance, it could be a good idea to also declare that 8 consecutive spaces anywhere in the code will be counted as equivalent to a tab. If there is ever a need for 8 literal spaces such a string can be programatically constructed.

I only allowed smaller closures in the code and refactored the rest into separate top-level functions. This is a deliberate move against the common practice of js programmers. Why? Because I noticed closures make code harder to read.

Very true; what precise requirements apply here?

I also decided there's no point refactoring large pieces of third-party code copied from elsewhere (e.g. the sha256 implementation). We should rather leave them as they are, at least until we have to make some bigger modifications to them.

The particular case of the SHA256 implementation has haunted my dreams far more than WebExtensions APIs ever have, without any particularly pressing reason no less. As it stands the only barrier to discarding it entirely is that the URL whitelisting system depends on it; NoScript seems to load whitelisting information asynchrously and then do some sort of "soft reload" instead, yet that seems quite complex to implement and solves something that isn't really a problem and doesn't need a solution at all. /rant

That has nothing to do with your decision anyway, which is entirely reasonable.

jahoti wrote:

the only thing missing is an explicit standard for loops/conditionals without braces.

Let's also follow Linux here - when the body of a loop/conditional fits within a single line, braces should be omitted. Exception is considered for a sequence of if-else-if blocks. If at least one of the blocks has braces, other ones should have them added, too. Also, an `else' should go at the same line a closing brace preceding it.

A more useful restriction in my opinion would be to avoid putting mixing code and comments on a line (which I solemnly swear not to do in future).

Interesting. What is the rationale for that restriction?

Indentation is ... This really sucks and I stuck to this simply because I was too lazy to change the defaults.

This works fine for me! However, as an additional layer of assurance, it could be a good idea to also declare that 8 consecutive spaces anywhere in the code will be counted as equivalent to a tab. If there is ever a need for 8 literal spaces such a string can be programatically constructed.

I don't think indentation rules should affect string contents... Also, it is uncommon to use literal tabs in strings. `\t' is used instead, so it would make more sense to treat 8 spaces in a string as what they are. Btw, I don't even imagine how we could change the meaning of string contents which are handled by javascript engine (I guess you actually didn't mean that?).

I only allowed smaller closures in the code and refactored the rest into separate top-level functions. This is a deliberate move against the common practice of js programmers. Why? Because I noticed closures make code harder to read.

Very true; what precise requirements apply here?

Avoiding closures with braces probably does the job.

I also decided there's no point refactoring large pieces of third-party code copied from elsewhere (e.g. the sha256 implementation). We should rather leave them as they are, at least until we have to make some bigger modifications to them.

The particular case of the SHA256 implementation has haunted my dreams far more than WebExtensions APIs ever have, without any particularly pressing reason no less. As it stands the only barrier to discarding it entirely is that the URL whitelisting system depends on it;

As of now. But we could make more use of it later. At some point we would even use cryptographic signatures to validate scripts. Not to mention possible other uses. I realize, though, the other uses could be satisfied by Crypto.subtle, while this one couldn't (due to the requirement to do everything synchronously).

Actually, the SHA256 is not strictly necessary here. It only makes it impossible to retrieve the actual secret from the unique value we use. As page's scripts shouldn't have access to that unique value anyway, we could stop using the hash. I think it is more secure to keep it, though.

NoScript seems to load whitelisting information asynchrously and then do some sort of "soft reload" instead, yet that seems quite complex to implement and solves something that isn't really a problem and doesn't need a solution at all. /rant

Indeed. I would be afraid it could make the unfrozen scripts misbehave.

Some more issues remain. For example, whitespace in object literals. But we can already make a draft of the convention and update it as necessary

Let's also follow Linux here - when the body of a loop/conditional fits within a single line, braces should be omitted. Exception is considered for a sequence of if-else-if blocks. If at least one of the blocks has braces, other ones should have them added, too. Also, an `else' should go at the same line a closing brace preceding it.

Sounds good!

A more useful restriction in my opinion would be to avoid putting mixing code and comments on a line (which I solemnly swear not to do in future).
Interesting. What is the rationale for that restriction?

Perhaps it's my inexperience showing; however, using inline comments with an 80-character line limit seems like a recipe for comments that blend in and inconsistent style (some comments inline, other similar comments on their own line only to avoid code + comment going over 80 characters).

Of course, that could be something that never actually ends up happening in practice, in which case none of that matters :).

I don't think indentation rules should affect string contents... Also, it is uncommon to use literal tabs in strings. `\t' is used instead, so it would make more sense to treat 8 spaces in a string as what they are.

Very true- such a broad rule about 8 spaces would at least need to be restricted before implementation, if it's even worth trying.

Btw, I don't even imagine how we could change the meaning of string contents which are handled by javascript engine (I guess you actually didn't mean that?).

Indeed I didn't, as much as that power would be nice. The rule was only meant as a formality, so that we could periodically replace 8 spaces in a row with a tab to make indentation uniform. On the other hand, it's always possible to only do that at the start of a line, or even just to allow either option.

Avoiding closures with braces probably does the job.

Seems reasonable enough.

As of now. But we could make more use of [the SHA256 module] later. At some point we would even use cryptographic signatures to validate scripts. Not to mention possible other uses. I realize, though, the other uses could be satisfied by Crypto.subtle, while this one couldn't (due to the requirement to do everything synchronously).

That is indeed the problem; unfortunately it took me two tries at rewriting the module using crypto.subtle to realize it, and another one to learn that js doesn't offer an easy way to synchronize "threads".

Actually, the SHA256 is not strictly necessary here. It only makes it impossible to retrieve the actual secret from the unique value we use. As page's scripts shouldn't have access to that unique value anyway, we could stop using the hash. I think it is more secure to keep it, though.

Agreed- couldn't scripts on whitelisted pages read it from the URL then, in fact? The only sensible approach, assuming it even works, would seem to be making the wrapper function around content/main.js asynchrous.

A more useful restriction in my opinion would be to avoid putting mixing code and comments on a line (which I solemnly swear not to do in future).
Interesting. What is the rationale for that restriction?

Perhaps it's my inexperience showing; however, using inline comments with an 80-character line limit seems like a recipe for comments that blend in and inconsistent style (some comments inline, other similar comments on their own line only to avoid code + comment going over 80 characters).

This explanation makes perfect sense to me :)

Of course, that could be something that never actually ends up happening in practice

Actually, it seems likely to happen

As of now. But we could make more use of [the SHA256 module] later. At some point we would even use cryptographic signatures to validate scripts. Not to mention possible other uses. I realize, though, the other uses could be satisfied by Crypto.subtle, while this one couldn't (due to the requirement to do everything synchronously).

That is indeed the problem; unfortunately it took me two tries at rewriting the module using crypto.subtle to realize it, and another one to learn that js doesn't offer an easy way to synchronize "threads".

Sad to hear you struggled that much :/

Actually, the SHA256 is not strictly necessary here. It only makes it impossible to retrieve the actual secret from the unique value we use. As page's scripts shouldn't have access to that unique value anyway, we could stop using the hash. I think it is more secure to keep it, though.

Agreed- couldn't scripts on whitelisted pages read it from the URL then, in fact?

No, it gets removed from there by content script at document_start, before they get to execute.

The only sensible approach, assuming it even works, would seem to be making the wrapper function around content/main.js asynchrous.

You mean the wrapper function added by build.sh? As you probably already noticed, this is not going to help with anything. Wherever `await' appears, it gives page the opportunity to load and execute its scripts

That is indeed the problem; unfortunately it took me two tries at rewriting the module using crypto.subtle to realize it, and another one to learn that js doesn't offer an easy way to synchronize "threads".
Sad to hear you struggled that much :/

At least I know now and have one less "feature" to discover on a deadline :).

couldn't scripts on whitelisted pages read it from the URL then, in fact?
No, it gets removed from there by content script at document_start, before they get to execute.

Oh- that's actually a neat trick! (IMO anyway) In hindsight it should've been obvious from the fact that the unique value never shows up in the URL bar, yet it's still just as exciting.

You mean the wrapper function added by build.sh? As you probably already noticed, this is not going to help with anything. Wherever `await' appears, it gives page the opportunity to load and execute its scripts

I didn't actually- thank you for explaining! In that case, as you said, the only sensible way of removing synchrous hashes would be to drop the unique value.

In case anyone's revisiting this thread - I recently changed my habits a bit and started writing new code with

• function body opening brace in the same line
• OO in some places where it seems to make sense

This should be more like how most of JS code out there is written.

I currently seem to be the only person actually committing extension code (😢), so at least such change won't be an inconvenience to anyone