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.

Sign up for GitHub

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

Jump to bottom

wip: global.js: convert to typescript #9996

Draft
wants to merge 9 commits into
base: master
Choose a base branch
from
Draft

wip: global.js: convert to typescript #9996

wants to merge 9 commits into from
+23,039 −3,949

Conversation

Minion3665
Copy link
Member

This supersedes #8221

@Minion3665 Minion3665 mentioned this pull request Sep 5, 2024
Closed
github-advanced-security[bot]
github-advanced-security bot found potential problems Sep 5, 2024
View reviewed changes
browser/src/global.ts
link.setAttribute('href', this.uriPrefix + 'device-mobile.css');
brandingLink.setAttribute(
'href',
this.brandingUriPrefix + theme_prefix + 'branding-mobile.css',

Check warning

Code scanning / CodeQL

DOM text reinterpreted as HTML Medium

DOM text
Loading
is reinterpreted as HTML without escaping meta-characters.
browser/src/global.ts
link.setAttribute('href', this.uriPrefix + 'device-tablet.css');
brandingLink.setAttribute(
'href',
this.brandingUriPrefix + theme_prefix + 'branding-tablet.css',

Check warning

Code scanning / CodeQL

DOM text reinterpreted as HTML Medium

DOM text
Loading
is reinterpreted as HTML without escaping meta-characters.
browser/src/global.ts
link.setAttribute('href', this.uriPrefix + 'device-desktop.css');
brandingLink.setAttribute(
'href',
this.brandingUriPrefix + theme_prefix + 'branding-desktop.css',

Check warning

Code scanning / CodeQL

DOM text reinterpreted as HTML Medium

DOM text
Loading
is reinterpreted as HTML without escaping meta-characters.
browser/src/global.ts
element.dataset.geolocationSetup.toLowerCase().trim() === 'true';
}

postMessageHandler(e) {

Check warning

Code scanning / CodeQL

Missing origin verification in `postMessage` handler Medium

Postmessage handler has no origin check.
browser/src/global.ts

window.ThisIsTheGtkApp = true;
window.postMobileMessage = function (msg) {
window.webkit.messageHandlers.cool.postMessage(msg, '*');

Check warning

Code scanning / CodeQL

Cross-window communication with unrestricted target origin Medium

Sensitive data
Loading
is sent to another window without origin restriction.
Sensitive data
Loading
is sent to another window without origin restriction.
Sensitive data
Loading
is sent to another window without origin restriction.
Sensitive data
Loading
is sent to another window without origin restriction.
Sensitive data
Loading
is sent to another window without origin restriction.
browser/src/global.ts
window.webkit.messageHandlers.error.postMessage(msg, '*');
};
window.postMobileDebug = function (msg) {
window.webkit.messageHandlers.debug.postMessage(msg, '*');

Check warning

Code scanning / CodeQL

Cross-window communication with unrestricted target origin Medium

Sensitive data
Loading
is sent to another window without origin restriction.
Sensitive data
Loading
is sent to another window without origin restriction.
Sensitive data
Loading
is sent to another window without origin restriction.
Sensitive data
Loading
is sent to another window without origin restriction.
Sensitive data
Loading
is sent to another window without origin restriction.
Sensitive data
Loading
is sent to another window without origin restriction.
browser/src/global.ts Fixed Show fixed Hide fixed
@Minion3665 Minion3665 force-pushed the private/skyler/push-ntqysoqu branch 5 times, most recently from 7ad6a87 to 5ed6be1 Compare September 11, 2024 12:27
github-advanced-security[bot]
github-advanced-security bot found potential problems Sep 11, 2024
View reviewed changes
browser/src/global.ts
const ab = new ArrayBuffer(binStr.length);
const bv = new Uint8Array(ab);
for (let i = 0, l = binStr.length; i < l; i++) {
bv[[i]] = binStr.charCodeAt(i);

Check warning

Code scanning / CodeQL

Implicit operand conversion Warning

This expression will be implicitly converted from object to string.
browser/src/slideshow/engine/ActivityFactory.ts
const height = aBBox.height / aActivityParamSet.nSlideHeight;
const x = (aBBox.x + aBBox.width / 2) / aActivityParamSet.nSlideWidth;
const y = (aBBox.y + aBBox.height / 2) / aActivityParamSet.nSlideHeight;
const width = aBBox!.width / aActivityParamSet.nSlideWidth;

Check notice

Code scanning / CodeQL

Unused variable, import, function or class Note

Unused variable width.
browser/src/slideshow/engine/ActivityFactory.ts
const x = (aBBox.x + aBBox.width / 2) / aActivityParamSet.nSlideWidth;
const y = (aBBox.y + aBBox.height / 2) / aActivityParamSet.nSlideHeight;
const width = aBBox!.width / aActivityParamSet.nSlideWidth;
const height = aBBox!.height / aActivityParamSet.nSlideHeight;

Check notice

Code scanning / CodeQL

Unused variable, import, function or class Note

Unused variable height.
browser/src/slideshow/engine/ActivityFactory.ts
const y = (aBBox.y + aBBox.height / 2) / aActivityParamSet.nSlideHeight;
const width = aBBox!.width / aActivityParamSet.nSlideWidth;
const height = aBBox!.height / aActivityParamSet.nSlideHeight;
const x = (aBBox!.x + aBBox!.width / 2) / aActivityParamSet.nSlideWidth;

Check notice

Code scanning / CodeQL

Unused variable, import, function or class Note

Unused variable x.
browser/src/slideshow/engine/ActivityFactory.ts
const width = aBBox!.width / aActivityParamSet.nSlideWidth;
const height = aBBox!.height / aActivityParamSet.nSlideHeight;
const x = (aBBox!.x + aBBox!.width / 2) / aActivityParamSet.nSlideWidth;
const y = (aBBox!.y + aBBox!.height / 2) / aActivityParamSet.nSlideHeight;

Check notice

Code scanning / CodeQL

Unused variable, import, function or class Note

Unused variable y.
@Minion3665
test: Add betterer
c999565 
Betterer is a snapshot test runner which tries to reduce the number of
errors. With, for example, eslint, we need to get to 0 errors for the
test to pass. This is prohibitive for large changes (e.g. turning on a
new eslint option), as you then have to make a lot of changes in the
same pull request. For betterer, you only have to avoid making the test
fail *more*, so it's OK to fix errors or leave things alone.

---

It runs as a part of check, and outputs (but doesn't block) building.

You can also run it manually, in the same way as prettier or eslint

$ make betterer

And get it to update its snapshots, again in the same way as prettier

$ make betterer-write

---

I avoided adding the betterer precommit, as running eslint takes a long
while and this blocked the commit for longer than I considered
acceptable. There's probably a way to make this work with eslint-d and a
custom test, but even so it's likely custom, brittle and non-ideal.

Signed-off-by: Skyler Grey <[email protected]>
Change-Id: I1770a1a8be3f45b45bda0fd2bcd57563add25924
@Minion3665
refactor(treewide): Enable strict null checking
24c631b 
Typescript defaults to treating null/undefined in a way that can leak
them throughout code, and will also cause type issues (unexpected
undefineds/anys) if you use them explicitly

To avoid an unreviewable mess I have either widened types to allow null
(where we own the type) or added non-null assertions (where we do not).
This is preferable to not discerning nulls at all, and while it still
creates a large (by LOC) change, it's hopefully not large (by variation
of things) - and should transpile identically to JavaScript.

There is one point where the code does not transpile identically to
JavaScript. This is because it directly set something that we don't own
which shouldn't be undefined to undefined. This is clearly incorrect,
and I've replaced it with setting to an empty string instead.

If it seems like this is bad practice, that's because it is. You
shouldn't add non-null assertions all over the place because it makes
typescript lie to you. It's of similar badness to adding `as x`
everywhere. Unfortunately, worse practice would be mandating that
typescript lie to you about nulls and undefineds everywhere, as our
tsconfig has been doing. It's far better to limit the lie to these ~850
places where I've had to explicitly tell typescript "the world isn't
actually like this - that's OK, carry on anyway".

If you're worried about blames from this change - first: I believe it's
worth it, second: this change should only affect single lines, so using
--ignore-rev should work very well on it

In the future, probably we should remove some of these assertions,
although some changes we'll have to make to remove them are likely
either tedious or dangerous.

Signed-off-by: Skyler Grey <[email protected]>
Change-Id: I1770a1a8be3f45b45bda0fd2bcd57563add25924
@Minion3665
refactor(global): rename global.js to global.ts
55b18eb 
The first step in adding typescript to global, we need to rename the
file. We also want to change every reference to global.js in this step,
as it will no longer exist afterwards...
@Minion3665
refactor(global): split eslint-disable
d85e1c7 
In the previous commit, I disabled all eslint rules. It may be nicer to
split these and handle them one-by-one
@Minion3665
refactor(global): eliminate 'this' confusion
5eb6b11 
'this' is often pretty hard to follow - especially in classes or
callback functions. We've previously inconsistently aliased it to 'that'
- but a much better solution is using arrow functions instead, which
always have 'this' from their surrounding scope

I've opted to replace all function() with arrow functions inside our
constructors for consistency, readability and avoidance of this
happening again - even if they don't strictly need it for their 'this'
uses to be correctly assigned
@Minion3665
refactor(global): convert arguments to ...args
8326f6e 
...args is a [rest parameter](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Functions/rest_parameters),
and has various advantages over using argments directly:

- it's an array, so there's no need for the difficult-to-read
  Array.prototype.slice.call(...) (*or its friendlier es6 successor,
  Array.from)
- it's easier to see at a glance where the arguments are coming from
- your IDE can give you a hint that the function is variadic - i.e. can
  take a variable number of arguments
@Minion3665
refactor(global): move Base64ToArrayBuffer to IIFE
616eaed 
For everything else, we're using an IIFE and assigning properties on its
argument to set global variables. To preserve consistency and allow us
to remove var later, it's nice to do the same.
@Minion3665
refactor(global): use const and let
3f3ba63 
let and const have different scoping and hoisting rules to var: they
only apply in the scope they were defined, and they are not hoisted

This makes it easier to see where variables are coming from
@Minion3665
refactor(global): remove IIFE
64043ac 
If we're using let and const, there's no need to wrap everything in a
function. Instead, we can use curly braces alone to create a scope
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
CODE "red"
Projects
Collabora Online
Status: To Review
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@Minion3665 @eszkadev