xpra icon
Bug tracker and wiki

Opened 3 months ago

Closed 3 months ago

#2705 closed enhancement (fixed)

js syntax

Reported by: brief Owned by: brief
Priority: trivial Milestone: 4.0
Component: html5 Version: 3.0.x
Keywords: Cc:

Description

changes:
reduced hints generally

  • use hasOwnProperty to check for attribute
  • remove duplicate jQuery request
  • inline variable

Thats why I found:

  • more missing semicolons
  • some missing declarations
  • some syntax errors
  • more vars to let/const

Attachments (14)

another.patch (68.0 KB) - added by brief 3 months ago.
1 semicolons.patch (6.7 KB) - added by brief 3 months ago.
2 let-const.patch (45.9 KB) - added by brief 3 months ago.
3 checkProperties.patch (5.1 KB) - added by brief 3 months ago.
4 duplicate-jquery-calls.patch (3.9 KB) - added by brief 3 months ago.
5 simplifications.patch (10.3 KB) - added by brief 3 months ago.
6 wrong me-this used.patch (1.3 KB) - added by brief 3 months ago.
7 add missing parameter.patch (853 bytes) - added by brief 3 months ago.
8 closing img does not exist.patch (1.1 KB) - added by brief 3 months ago.
9 reserved keyword.patch (567 bytes) - added by brief 3 months ago.
10 adjust type.patch (1.1 KB) - added by brief 3 months ago.
11 wrong url.patch (370 bytes) - added by brief 3 months ago.
12 language selection.patch (401 bytes) - added by brief 3 months ago.
final.patch (14.3 KB) - added by brief 3 months ago.

Download all attachments as: .zip

Change History (27)

Changed 3 months ago by brief

Attachment: another.patch added

comment:1 Changed 3 months ago by brief

Component: androidhtml5
Priority: majortrivial
Type: defectenhancement

Should I start a new ticket to discuss linting?
The non consistent syntax makes creating patches without withspace changes very hard.

comment:2 Changed 3 months ago by Antoine Martin

Owner: changed from Antoine Martin to brief

I don't doubt that some of these changes may well be valid, but I can't just apply them blindly. Can you comment?

I would much prefer removing "id_XXXX":

  • const pasteboard = $("#pasteboard");
  • skipped this, as it looked wrong:
    if (window.hasOwnProperty("clipboardData") && window.clipboardData.hasOwnProperty("setData") && typeof window.clipboardData.setData === "function") {
    

=== "function" will never match?
Same in:
else if (CLIPBOARD_IMAGES && dtype=="image/png" && dformat==8 && wire_encoding=="bytes" && navigator.clipboard && navigator.clipboard.write) {

  • I didn't really see the point of most of these hasOwnProperty change:
    clog("file:", f.name, ", type:", f.type, ", size:", f.size, "last modified:", f.hasOwnProperty("lastModifiedDate") ? f.lastModifiedDate.toLocaleDateString() : 'n/a');
    
    if (CLIPBOARD_IMAGES && navigator.clipboard && navigator.clipboard.hasOwnProperty("write")) {
    
    navigator.hasOwnProperty("connection")
    

etc..
What sort of problems is this meant to prevent?

  • rename const id_screen to const screen and id_float_menu to float_menu
  • //args = args.splice(1); - why was this commented out?
  • these look like bug fixes:
    • this._do_connect(false);
    • packet = ["key-action", me.topwindow, keyname, false, modifiers, keyval, str, keycode, group];
    • this.protocol = null;
    • const width = parseInt(this.video.getAttribute("width"));
  • in _tray_geometry_changed, _tray_set_focus and _tray_closed, why add the ctx argument ?
  • why change the type of the notification_icon element?
  • rename class_notifications to notifications.
  • const wid = jQuery("#"+String(this.wid)); - wid has a very specific meaning in xpra, that's the window id, please rename this object to window

Also, please try to keep indentation / semi-colon type of patches separate from the changes that will modify behaviour.
We can apply the former very quickly, as it is quick to review, the latter may take more time.

comment:3 Changed 3 months ago by Antoine Martin

Should I start a new ticket to discuss linting?

I don't mind.

The non consistent syntax makes creating patches without whitespace changes very hard.

We can merge one change that only changes whitespace and nothing else.
Then things will be easier?

comment:4 in reply to:  2 Changed 3 months ago by brief

Replying to Antoine Martin:

I don't doubt that some of these changes may well be valid, but I can't just apply them blindly. Can you comment?

I would much prefer removing "id_XXXX":

  • const pasteboard = $("#pasteboard");

So remove the id_ part of variables? Or dont remove duplicate jQuery calls?

  • skipped this, as it looked wrong:
    if (window.hasOwnProperty("clipboardData") && window.clipboardData.hasOwnProperty("setData") && typeof window.clipboardData.setData === "function") {
    

=== "function" will never match?

typeof window.clipboardData.setData === "function"
will match

Same in:
else if (CLIPBOARD_IMAGES && dtype=="image/png" && dformat==8 && wire_encoding=="bytes" && navigator.clipboard && navigator.clipboard.write) {

  • I didn't really see the point of most of these hasOwnProperty change:


clog("file:", f.name, ", type:", f.type, ", size:", f.size, "last modified:", f.hasOwnProperty("lastModifiedDate") ? f.lastModifiedDate.toLocaleDateString() : 'n/a');
if (CLIPBOARD_IMAGES && navigator.clipboard && navigator.clipboard.hasOwnProperty("write")) {
navigator.hasOwnProperty("connection")

etc..
What sort of problems is this meant to prevent?

With clipboardData.setData the attribute is accessed, clipboardData.hasOwnProperty("setData") checks if there is such an attribute (what was meant with clipboardData.setData). On expecting a function, typeof window.clipboardData.setData === "function" needs to be used.
It results in no access to undefined objects (and also no warnings).

  • rename const id_screen to const screen and id_float_menu to float_menu
  • //args = args.splice(1); - why was this commented out?

It is commented out to point out that it is not used, can be deleted.

  • these look like bug fixes:
    • this._do_connect(false);
    • packet = ["key-action", me.topwindow, keyname, false, modifiers, keyval, str, keycode, group];
    • this.protocol = null;
    • const width = parseInt(this.video.getAttribute("width"));

Yes

  • in _tray_geometry_changed, _tray_set_focus and _tray_closed, why add the ctx argument ?

ctx is used in the line below like ctx.debug("tray", "tray geometry changed (ignored)");

  • why change the type of the notification_icon element?

This is an error. My intention was to only delete the closing img tag (</img>) as this does not exists.

  • rename class_notifications to notifications.

I wanted to get rid of the duplicate jQuery call, so I introduced a variable class_notifications.

  • const wid = jQuery("#"+String(this.wid)); - wid has a very specific meaning in xpra, that's the window id, please rename this object to window

window is in used by JS, what about window_element?

Also, please try to keep indentation / semi-colon type of patches separate from the changes that will modify behaviour.
We can apply the former very quickly, as it is quick to review, the latter may take more time.

Changed 3 months ago by brief

Attachment: 1 semicolons.patch added

Changed 3 months ago by brief

Attachment: 2 let-const.patch added

Changed 3 months ago by brief

Attachment: 3 checkProperties.patch added

Changed 3 months ago by brief

Changed 3 months ago by brief

Attachment: 5 simplifications.patch added

Changed 3 months ago by brief

Attachment: 6 wrong me-this used.patch added

Changed 3 months ago by brief

Changed 3 months ago by brief

Changed 3 months ago by brief

Attachment: 9 reserved keyword.patch added

Changed 3 months ago by brief

Attachment: 10 adjust type.patch added

Changed 3 months ago by brief

Attachment: 11 wrong url.patch added

Changed 3 months ago by brief

Attachment: 12 language selection.patch added

comment:5 Changed 3 months ago by brief

Like this?
These are not incremental, is this even possible? Do you create a separate branch on merging?

comment:6 Changed 3 months ago by Antoine Martin

Like this?

Perfect, thank you.

Updates:

  • r25942 semi-colons
  • r25943 simplify (partial: only real simplifications in this changeset)
  • r25944 me vs this
  • r25946 more correct fix for tray geometry / focus / closed functions (we can't just add ctx to the method, or use this, the window calls the geometry callback using: this.geometry_cb(this);.
  • r25947 don't close img tag
  • r25948 don't use the 'char' reserved keyword
  • r25949 lineWidth is a number
  • r25950 clipboardData -> window.clipboardData
  • r25951 cursor URL
  • r25952 language string matching

Not done yet:

  • let-const as we may have to revert all of those changes to fix the minifier (uglifyjs does not support ES6 - maybe use terser instead?)
  • check properties using hasOwnProperty - avoiding warnings is nice, the syntax is not
  • duplicate jquery calls: will do

Only problem is that something broke and makes the html5 client unusable:

Uncaught ReferenceError: Buffer is not defined
    at Object.<anonymous> (forge.js:25985)
    at __webpack_require__ (forge.js:30)
    at Object.<anonymous> (forge.js:25412)
    at __webpack_require__ (forge.js:30)
    at Object.<anonymous> (forge.js:25391)
    at __webpack_require__ (forge.js:30)
    at forge.js:73
    at forge.js:76
    at webpackUniversalModuleDefinition (forge.js:9)
    at forge.js:10

comment:7 Changed 3 months ago by Antoine Martin

Uncaught ReferenceError: Buffer is not defined

Fixed in r25953.

comment:8 Changed 3 months ago by Antoine Martin

Updates:

  • r25954 const / let
  • r25955 fixes paint debugging (rather than using hasOwnProperty for something which is never defined)
  • still about hasOwnProperty: I'm going to hold off on this one:
    navigator.clipboard.write
    ƒ () { [native code] }
    navigator.clipboard.write === "function"
    false
    navigator.clipboard.write == "function"
    false
    
  • r25956 removes duplicate !jQuery calls
Version 0, edited 3 months ago by Antoine Martin (next)

comment:9 Changed 3 months ago by Antoine Martin

r25965 fixes uglifyjs using regexp substitutions, so we lose the "const" and "let" changes during minification... This will have to do until we switch to terser (#2709).

comment:10 Changed 3 months ago by Antoine Martin

@brief: can I close this or do you want to add more?

Changed 3 months ago by brief

Attachment: final.patch added

comment:11 in reply to:  8 Changed 3 months ago by brief

This should be the last one.

Replying to Antoine Martin:

  • still about hasOwnProperty: I'm going to hold off on this one:
    navigator.clipboard.write
    ƒ () { [native code] }
    navigator.clipboard.write === "function"
    false
    navigator.clipboard.write == "function"
    false
    

I only made one patch now since this looks like a line mismatch while merging.

comment:12 Changed 3 months ago by Antoine Martin

Thanks!

From the "final" patch:

  • r26072 adds the missing semi-colons
  • r26073 just removes lastModifiedDate
  • r26074 use hasOwnProperty

As per comment:8, the check for === "function" does not work under chrome, so I have ignored that.
Also, I am not merging the const Buffer and const LZ4 because of the problems this causes, see comment:7 (though the problem may not show since we replace const by var before using the minifier - nonetheless, AFAICT this is still a problem)

comment:13 Changed 3 months ago by Antoine Martin

Resolution: fixed
Status: newclosed

Closing, let's continue this in a new ticket for 4.1

Note: See TracTickets for help on using tickets.