-
Notifications
You must be signed in to change notification settings - Fork 56
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
update window.browser spec per #532 #546
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Patrick! I left a review with some wording suggestions. I know there was some concern in the last meeting about if it made sense to do this (and I'm still not fully sure) so I think we should get approval from @Rob--W and others before deciding if we merge.
@oliverdunk thanks for the review! I believe I addressed everything in the last update, so another look at some point would be appreciated! @Rob--W I'd love any feedback you have |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Besides the review, another thought related to your comment at #532 (comment)
Whether or not
chrome.foo = "bar"
would automatically makebrowser.foo === "bar"
is left up to the user agent to decide. (With our proposal, this would be false. My understanding was that Firefox was fine with changing to match behavior, but Safari was not interested in changing their strict aliasing as they didn't have anything onchrome
they didn't also want onbrowser
)
The thought occurred to be that there may be little point in distinguishing the browser
and chrome
namespaces. Any existing property of chrome
is de facto reserved, because if another browser were to introduce a new API under browser.loadTimes
, for example, then developers would be surprised when they use chrome.loadTimes
with the expectation of browser.loadTimes
pointing to the same object. browser
as a namespace explicitly permits UA-specific additions, which is good enough to allow Chrome to expose chrome.csi
as browser.csi
, but as a browser-specific "extension" API. Thoughts on this one?
Co-authored-by: Rob Wu <[email protected]>
Sorry, I was under the impression I was just approving the typo commit, not this whole PR.
I need to find some time still to comment on this as a whole.
Co-authored-by: Rob Wu <[email protected]>
Co-authored-by: Rob Wu <[email protected]>
@Rob--W My thought process was that a developer may be surprised to see that modifying |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since Chrome is unwilling to have chrome === browser
that Safari and Firefox already supports, we will spec the "not direct alias" behavior.
// As `chrome` and `browser` are not direct aliases, | ||
// modifying one top level values will not change the other |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// As `chrome` and `browser` are not direct aliases, | |
// modifying one top level values will not change the other | |
// When `chrome` and `browser` are not direct aliases, | |
// modifying one top level values will not change the other: |
Rephrased to avoid the impression that chrome and browser couldn't be direct aliases.
// modifying one top level values will not change the other | ||
|
||
globalThis.browser.FAKE = true | ||
console.log(globalThis.chrome.FAKE); //undefined |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
console.log(globalThis.chrome.FAKE); //undefined | |
console.log(globalThis.chrome.FAKE); | |
// ^ undefined when chrome and browser are not direct aliases. | |
// ^ may be true if browser === chrome. |
Fixes #532