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

ExecutionWorld and StyleOrigin should use lowercase values #563

Open
xeenon opened this issue Mar 9, 2024 · 14 comments
Open

ExecutionWorld and StyleOrigin should use lowercase values #563

xeenon opened this issue Mar 9, 2024 · 14 comments
Labels
follow-up: chrome Needs a response from a Chrome representative inconsistency Inconsistent behavior across browsers proposal Proposal for a change or new feature spec clarification Needs clarification when specified supportive: firefox Supportive from Firefox supportive: safari Supportive from Safari

Comments

@xeenon
Copy link
Collaborator

xeenon commented Mar 9, 2024

At present, ExecutionWorld requires values to be uppercase, specifically MAIN or ISOLATED. Similarly, StyleOrigin also requires the uppercase values AUTHOR or USER. This approach, for these newer APIs, is in contrast to the convention used for older properties, such as RunAt, which uses lowercase values like document_start or document_idle.

To improve consistency across the Web Extensions API, I recommend standardizing in favor of lowercase for these values. Additionally, for backward compatibility, browsers should adopt a case-insensitive comparison for these values until the uppercase values are sufficiently phased out.

Moving forward, I suggest the adoption of lowercase string constants in all future APIs, to establish a uniform standard.

@xeenon xeenon added spec clarification Needs clarification when specified supportive: safari Supportive from Safari proposal Proposal for a change or new feature needs-triage: chrome Chrome needs to assess this issue for the first time needs-triage: firefox Firefox needs to assess this issue for the first time labels Mar 9, 2024
@erosman
Copy link

erosman commented Mar 9, 2024

Original MV2 tabs.insertCSS(): cssOrigin & CSSOrigin also used lowercase "user" & "author".

@rdcronin
Copy link
Contributor

I agree with deciding on a uniform standard.

ExecutionWorld is not the first (or by any means, only) API to use SCREAMING_STYLE for the enum values. We have historically been very inconsistent about this, using combinations of SCREAMING_STYLE, camelCaseStyle, and snake_case_style. We eventually somewhat standardized in Chrome on SCREAMING_STYLE because these were similar to enums or constants, both of which are represented with SCREAMING_STYLE in the Google JS Style Guide, and is also the referenced "common convention" in MDN to represent constants -- though of course, MDN would never require a certain style.

I would prefer aligning on using all-caps to better fit with these existing patterns and conventions.

Note also that case-insensitivity only works between SCREAMING_STYLE and snake_case_style; it does not solve the issue for camelCaseStyle. Several years back, to help avoid this issue, Chrome began exposing enums on the APIs themselves, a la chrome.runtime.ContextType.BACKGROUND -- these are always SCREAMING_STYLE, even when the mapped string value is not (chrome.runtime.RequestUpdateCheckStatus.NO_UPDATE evaluates to 'no_update').

@tophf
Copy link

tophf commented Mar 13, 2024

I would prefer aligning on using all-caps to better fit with these existing patterns and conventions.

No, you've introduced an inconsistency because values in the API have always been in lowercase/camelCase. Even the new declarativeNetRequest API uses lowercase/camelCase for the values e.g. "type" : "block". You also weirdly equate the rules for naming of a constant to its value, which is not regulated by Google JS Style Guide you've linked, AFAICT, or any other JS guide I know for that matter.

@xeenon
Copy link
Collaborator Author

xeenon commented Mar 13, 2024

@rdcronin I agree SCREAMING_STYLE should be used for the JS properties representing the constants, and having the string values be lowercase.

@dotproto
Copy link
Member

@rdcronin I agree SCREAMING_STYLE should be used for the JS properties representing the constants, and having the string values be lowercase.

I read @rdcronin's comment as suggested SCREAMING_STYLE for enums and constants, and suggesting that we align on uppercase to match Chrome's current conventions.

You also weirdly equate the rules for naming of a constant to its value, which is not regulated by Google JS Style Guide you've linked, AFAICT, or any other JS guide I know for that matter.

That doesn't seem odd to me at all. Automated tooling can easily explain how coding style can directly impact implementation details. For example, TypeScript enums auto-assign numeric and string properties based on the order of member declaration and the member's key.

// TypeScript source
enum Shape {
    CIRCLE,
    SQUARE,
}

// Compiled JS
var Shape;
(function (Shape) {
    Shape[Shape["CIRCLE"] = 0] = "CIRCLE";
    Shape[Shape["SQUARE"] = 1] = "SQUARE";
})(Shape || (Shape = {}));

console.log(Shape[0], Shape.CIRCLE); // "CIRCLE"
console.log(Shape.CIRCLE) // 0

@tophf
Copy link

tophf commented Mar 14, 2024

suggesting that we align on uppercase to match Chrome's current conventions.

It doesn't seem practical or reasonable, because just a handful of values are in uppercase in a couple of new API, whereas hundred(s) of the values are in lowercase.

That doesn't seem odd to me at all. Automated tooling can easily explain

Linking a style guide for naming was unequivocally odd. Your argument doesn't stand either as automated tools can easily do whatever we decide, i.e. automation is irrelevant, moreover Chromium already has the tooling to generate the uppercase names from lowercase values.

I'd also like to point out that despite @rdcronin's comment this issue is not about "impassioned bikeshed discussions". It's about an objectively existing problem introduced by chromium team, apparently while trying to improve consistency but ending with an anecdotally (xkcd) new inconsistency.

@Rob--W Rob--W added supportive: firefox Supportive from Firefox and removed needs-triage: firefox Firefox needs to assess this issue for the first time labels Mar 14, 2024
@Rob--W
Copy link
Member

Rob--W commented Mar 14, 2024

supportive: firefox Supportive from Firefox for having consistency across APIs. The specifics (i.e. whether lowercase or uppercase) is a lesser concern.

@tophf
Copy link

tophf commented Mar 14, 2024

The specifics (i.e. whether lowercase or uppercase) is a lesser concern.

From a practical point of view, this concern is just as important, because if the decision will be to use the uppercase for values it would mean changing a lot of existing values (possibly hundreds) across multiple APIs, which seems entirely unrealistic. I do understand that expecting chromium team to admit they made a mistake is not entirely realistic either. (Edit: looks like it's not that clear-cut). The most realistic solution would be to ignore the problem, of course.

@Rob--W
Copy link
Member

Rob--W commented Mar 14, 2024

An easily actionable task here is to reach consensus on the supported value format for new APIs.

Secondarily, we could consider changes to existing APIs that deviate from the common conventions, but I wouldn't require that.

@rdcronin
Copy link
Contributor

Just to be clear: we have never been consistent; this wasn't something that Chrome just recently changed. Uppercase values were introduced more than a decade ago (one easy example).

Today, non-trivial numbers of enum entries are:

  • All uppercase
  • All lowercase
  • Mixed case (though a good chunk of these are all from the massive fontSettings.ScriptCode type)

Less common variations include enums with hyphens (8) and enums with number (12).

The web itself is also inconsistent. While many favor lowercase (and there, they use a lot more hyphens than underscores), there are also examples of uppercase enums (e.g., GET and POST). However, most browsers today allow case-insensitive comparison of these, which is inline with @xeenon 's recommendation in the issue summary.

An easily actionable task here is to reach consensus on the supported value format for new APIs.

Agreed. @xeenon , do you mind if we repurpose this issue's title to target that?

@tophf
Copy link

tophf commented Mar 15, 2024

Okay, so Chromium did a sensible thing to always name them in SCREAMING_CASE in those original IDLs, it's not what the issue is about and anyway those historically uppercase values are scattered in rarely used APIs/properties, so my earlier claim should be corrected to say that almost all APIs used by most extensions have all their values in lowercase/camelCase:

  • content scripts e.g. document_start
  • contextMenus e.g. image, checkbox
  • declarativeNetRequest, webRequest e.g. main_frame
  • cookies e.g. expired, lax
  • extension e.g. tab, popup
  • management e.g. sideload, theme
  • notifications e.g. image, basic
  • omnibox e.g. url, match, dim
  • proxy e.g. fixed_servers
  • runtime e.g. install, update, arm64, os_update, android, update_available
  • tabs e.g. capture, unloaded, popup, automatic, per-origin
  • webNavigation e.g. form_submit
  • webRequest e.g. request_headers, asyncBlocking, xmlhttprequest
  • windows e.g. normal

Isn't it obvious that a lowercase value is the most rational choice?

Note how runtime had 6 lowercase enums with dozens of lowercase values and suddenly it receives a new ContextType enum with all uppercase values, which looks objectively weird, unusual, and wrong.

Arguably, if we scan the source code of all existing extensions for literal enum values like "document_start" and exclude the recently introduced uppercase values, close to 99% of them will be lowercase.

@dotproto
Copy link
Member

While looking into WebIDL for unrelated reason, I came across the following quotation after the definition of enumeration values:

Warning!

It is strongly suggested that enumeration values be all lowercase, and that multiple words be separated using dashes or not be separated at all, unless there is a specific reason to use another value naming scheme. For example, an enumeration value that indicates an object should be created could be named "createobject" or "create-object". Consider related uses of enumeration values when deciding whether to dash-separate or not separate enumeration value words so that similar APIs are consistent.

@oliverdunk oliverdunk added follow-up: chrome Needs a response from a Chrome representative and removed needs-triage: chrome Chrome needs to assess this issue for the first time labels Apr 23, 2024
@rdcronin
Copy link
Contributor

rdcronin commented Jul 3, 2024

Circling back to this one.

Chrome has been investigating how we can shift our IDL to move away from the current syntax (which is non-standard IDL and has a lot of issues, including not being able to specify promises). I think this will best be solved going forward by aligning on that syntax (there will be a separate issue filed for that).

The tentative plan for enumerations in that new syntax looks like:

enum WindowState {"normal", "minimized", "maximized", "fullscreen", "locked-fullscreen"};

bringing us more inline with the web versions.

I'm also supportive of using case-insensitive matching, which should be a non-breaking change. If we wanted, we could also do a _ -> '-' conversion. This would allow us to have all of our docs and IDL specifications use the new syntax and styling while not breaking any existing extensions. Out of principle, I'd be hesitant to remove _ (e.g. LOCKED_FULLSCREEN to LOCKEDFULLSCREEN), since that theoretically raises the risk of collision -- even if I don't think there are any cases where it's an issue today.

For concrete steps, I'd propose:

  1. We finalize and implement the new schema. Any new APIs using this schema will use the new schema style.
  2. We implement handling for converting old values.
  3. We update old schemas (JSON and IDL) to new schemas that use the new enum values.

One risk is that the conversion would only happen in new versions of the browser, so if we update the documentation to match the new style, an extension may break if it tries to use that on an older browser. We may want to wait until the conversion is in all currently-supported versions before updating the enum style for documentation clarity, or otherwise tag those entries.

@xeenon , thoughts?

@xeenon
Copy link
Collaborator Author

xeenon commented Jul 3, 2024

@rdcronin That all sounds good to me!

@dotproto dotproto added the inconsistency Inconsistent behavior across browsers label Sep 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
follow-up: chrome Needs a response from a Chrome representative inconsistency Inconsistent behavior across browsers proposal Proposal for a change or new feature spec clarification Needs clarification when specified supportive: firefox Supportive from Firefox supportive: safari Supportive from Safari
Projects
None yet
Development

No branches or pull requests

7 participants