if this 'uglifyify' dependency is to be used there should be another guard above such as:

  which uglifyify > /dev/null || (echo "can not find uglifyify" && false) || exit

can you explain more about what particular problem these patches solve or which features they are adding - or are they all purely refactoring?

i agree that this program is in dire need of refactoring; but patches that solve a problem, patches that add a feature, and patches that merely refactor should be all separated from each other

i would particularly like to ask why minification is desirable but this PR seem that it addressed multiple concerns

can you explain more about what particular problem these patches solve or which features they are adding - or are they all purely refactoring?

The specific concern addressed by this PR is "Fixing the loading mechanism", which had multiple bugs:

• it didn't discriminate among different types of non-HTML resources loaded in windows/frames, e.g. images, potentially breaking their rendering
• it did not handle sub frames
• it did not handle charsets different than UTF-8

The fix is implemented through a refactoring which extracts out the HTTP response processing machinery into a separate module (ResponseProcessor): this preemptively skips responses we don't want to process and provides content-type / charset management.

The minification support was added because this and other work to come in this phase (e.g. to fix blacklisting/whitelisting) is adding source files whose growth could make it desirable; but I agree, it might better go in a different PR.
However, regarding the guard you suggest to add for uglifyify, since it's used as a browserify transform, "which" will generally fail. Nevertheless we'll better exit on any browserify error (either triggered by a missing dependency or by other problems) before packaging the XPI. Browserify is verbose enough about its errors, so this will likely suffice:

browserify $OPTS main_background.js -o bundle.js || exit

On Sun, 15 Jul 2018 23:06:19 +0000 (UTC) Gorgio wrote:

The fix is implemented through a refactoring

that is where i must object - refactoring by definition should never change
the behavior of the program - it really helps future readers of the commit
history to separate those concerns - it should always be possible to do the refactoring first without changing behavior and then in another commit on top of that making the fix that changes behavior - they could probably be included in the same PR; but separate commits would make the diffs much more
clear semantically

On Sun, 15 Jul 2018 23:06:19 +0000 (UTC) Gorgio wrote:

The minification support was added because this and other work to come in
this phase
is adding source files whose growth could make it
desirable; but I agree, it might better go in a different PR.

it should go in a separate PR because it is a separate feature - but more-so
because i would contend that minification is undesirable - it is a fairly
minor issue either way; but the point can be argued that there is little value in it - this is a tiny program (137K according to mozilla) so there is little savings to be gained in terms of network bandwidth, drive space, memory usage, or anything else - leaving all the code in human readable form allows users to read the code they already have in the .xpi without downloading the upstream source

On Sun, 15 Jul 2018 23:06:19 +0000 (UTC) Gorgio wrote:

However, regarding the guard you suggest to add for uglifyify,
we'll better exit on any browserify error

i think it is better engineering to be as clear and pedantic as possible in
such cases - that extra LOC hurts nothing; but makes it clear that the error was not a program bug but was a user error

regarding refactoring - i would also invite you to notice some of the "feature" branches that exist in this repo such as:

• 'comments' - for more thorough commenting of the source
• 'housekeeping' - for anything else that does not change behavior (refactoring)
• 'useless-files' - just because there were a lot of them

those were intended as i described above to separate different concerns - it is usually best not to ever target the master branch in a merge request; but to always target one such feature-specific development branch - feel free to add more branches if there are other general concerns or features

I would also prefer to not minify the code, the savings will likely be small since the xpi is compressed anyway, and shipping readable code is more desirable than the size saved, in particular due to the topic at hand.

I would also prefer to not minify the code, the savings will likely be small since the xpi is compressed anyway, and shipping readable code is more desirable than the size saved, in particular due to the topic at hand.

Fair. I reverted that commit and added the renamings requested by RMS to avoid dubious usages of the word "content".

I've tested the new loading mechanism and it is a huge improvement! I have a small patch which corrects the logic applied to external scripts, I'll send it your way after this set is merged.

Things to improve on the PR:

• On the two new files (bg/*) I think only your copyright line is necessary as they don't derive from code by Nathan or me.
• There are a lot of changes to remove trailing spaces, in the same commit as functional changes, they should be separate.
• commit https://pagure.io/fork/gioma1/librejs/c/a25ff071bb9c828954019be8c18a291639845654 has changes to the build.sh that you later revert, it would make more sense to correct that initial commit.

Notes for future tasks (no need to do them as part of this PR):

• It would be good to unify the indentation styles (there is a mix of tabs and spaces) as a future task.
• I think scripts that are to be part of the bundle should be all put together instead of having bg, evaluation_hash, nontrivial_utility, and scripts on the root dir. It makes it confusing to know where to look for things.

I've tested the new loading mechanism and it is a huge improvement! I have a small patch which corrects the logic applied to external scripts, I'll send it your way after this set is merged.
Things to improve on the PR:
[...]

I've implemented your suggestions and rebased my patch, merging also yours about external scripts as agreed on IRC.