LGTM3 On Wed, Nov 23, 2022 at 11:28 PM Chris Harrelson <[email protected]> wrote:
> LGTM2 > > On Wed, Nov 23, 2022, 4:14 PM Rick Byers <[email protected]> wrote: > >> Thanks for doing this analysis. According to our guidelines >> <https://docs.google.com/document/d/1RC-pBBvsazYfCNNUSkPqAVpSpNJ96U8trhNkfV0v9fk/edit#heading=h.4k9u8ddizqrq> >> this >> is in the low risk range of <0.001% of pages in HTTP Archive. Personally I >> think we should just go for it, but be prepared to revert (and rename the >> interface instead) if we get even a single bug report in beta. LGTM1 >> >> On Wed, Nov 23, 2022 at 4:50 PM Ian Clelland <[email protected]> >> wrote: >> >>> So I've run through HTTPArchive with these queries (shown for desktop, >>> mobile is analogous): >>> >>> SELECT APPROX_COUNT_DISTINCT(pageid) >>> FROM `httparchive.summary_requests.2022_10_01_desktop` AS REQ >>> JOIN `httparchive.response_bodies.2022_10_01_desktop` AS RESP on REQ.url >>> = RESP.url >>> WHERE ( >>> REQ.resp_content_type = 'application/javascript' OR >>> REQ.resp_content_type = 'text/javascript' OR >>> REQ.resp_content_type = 'application/html' OR >>> REQ.resp_content_type = 'text/html') >>> AND ( >>> REGEXP_CONTAINS(RESP.body, r"\bwindow\.Reporting\b") OR >>> REGEXP_CONTAINS(RESP.body, r"\bself\.Reporting\b")); >>> >>> >>> And found 161 pages on desktop (out of ~10M pages), and 185 on mobile >>> (of ~15M). (For comparison, replacing "Report" with "ReportingObserver", >>> yields ~123k pages on desktop referring to that symbol) >>> >>> Coalescing distinct resource URLs in this case (as many may be from the >>> same third-party library) yields just 110 URLs on mobile, including many >>> clearly repeated resources hosted on different subdomains of kindful.com, >>> and >>> several named simply '/js/common.js': >>> >>> >>> https://cookerevivals.kindful.com/assets/application-b1789711faae06cb6b1679d08f18c4ee8bfde712fa6375641c5e38fca8231494.js >>> >>> https://atlast.kindful.com/assets/application-b1789711faae06cb6b1679d08f18c4ee8bfde712fa6375641c5e38fca8231494.js >>> >>> https://indushospitalca.kindful.com/assets/application-b1789711faae06cb6b1679d08f18c4ee8bfde712fa6375641c5e38fca8231494.js >>> >>> https://stridereducationfoundation-bloom.kindful.com/assets/application-b1789711faae06cb6b1679d08f18c4ee8bfde712fa6375641c5e38fca8231494.js >>> (And many more; these ones unconditionally overwrite Report) >>> >>> https://tesotunes.com/js/common.js?version=2.1.5.8-beta-22 >>> http://arabgramz.com/js/common.js?x=37 >>> https://www.yavedo.com/js/common.js?x=55 >>> https://shakulive.com/js/common.js?x44 >>> (These ones do check for Report's existence, and use "window.Report || ( >>> window.Report = {} );" before defining new methods. This will also continue >>> to work, but will add new methods to the interface object, rather than the >>> presumed empty dict.) >>> >>> On Wed, Nov 23, 2022 at 12:48 PM Rick Byers <[email protected]> wrote: >>> >>>> >>>> On Wed, Nov 23, 2022 at 12:22 PM Yoav Weiss <[email protected]> >>>> wrote: >>>> >>>>> >>>>> On Wed, Nov 23, 2022 at 6:13 PM Ian Clelland <[email protected]> >>>>> wrote: >>>>> >>>>>> Well, that is exactly the situation outlined as a risk. I'm still >>>>>> running some HTTPArchive queries, but didn't immediately see much there. >>>>>> >>>>>> I expect that +Domenic Denicola <[email protected]> may have more >>>>>> context on the changes here, but my understanding is that WebIDL has been >>>>>> moving towards this for some time. >>>>>> >>>>>> The issues I'm aware of are on WebIDL: >>>>>> https://github.com/whatwg/webidl/issues/350 renames >>>>>> "NoInterfaceObject" to "LegacyNoInterfaceObject" >>>>>> https://github.com/whatwg/webidl/issues/365 starts mandating the use >>>>>> of "Exposed" in all interfaces >>>>>> >>>>>> Reporting specifically was updated in 2019, with >>>>>> https://github.com/w3c/reporting/pull/173, after >>>>>> https://github.com/whatwg/webidl/pull/423 made Exposed mandatory. >>>>>> >>>>> >>>>> Is it possible to rename these exposed interfaces to something less >>>>> generic, that's less likely to collide? >>>>> >>>> >>>> Since it's not exposed today, nobody should be depending on the name. >>>> So I'm sure we could. >>>> >>>> But I'd like to understand why we should expose it at all. I tried >>>> digging through the history of discussions on this topic and the contents >>>> of the WebIDL spec but couldn't get clarity on what the thinking has been. >>>> I filed https://github.com/whatwg/webidl/issues/1236 to hopefully have >>>> that captured somewhere. >>>> >>>> On Wed, Nov 23, 2022 at 11:18 AM Rick Byers <[email protected]> >>>>>> wrote: >>>>>> >>>>>>> I did a quick GitHub search and quickly found this >>>>>>> <https://github.com/xuehuibo/MobileQueryPlatform/blob/master/MobileQueryPlatform/MobileQueryPlatform/Views/Home/ReportMenu.cshtml>: >>>>>>> :-( >>>>>>> >>>>>>> if (window.Report == undefined) { >>>>>>> Tools.Namespace.register("window.Report"); >>>>>>> } >>>>>>> >>>>>>> And then a bunch of downstream code expecting to build stuff up on >>>>>>> window.Report. I didn't try to find out where or how much this is used, >>>>>>> but >>>>>>> it's certainly not promising. >>>>>>> >>>>>>> Where is this decision to expose all interface objects by default >>>>>>> coming from? Are we sure it's the right thing for the web when the >>>>>>> global >>>>>>> namespace is shared between the platform and application code? >>>>>>> >>>>>>> Sorry Ian, I know I said I thought this should be a simple one :-). >>>>>>> If we have data that it looks very rare, then I'm still happy to >>>>>>> approve. >>>>>>> I'm just wondering now about the bigger picture of this class of >>>>>>> changes. >>>>>>> >>>>>>> Rick >>>>>>> >>>>>>> On Wed, Nov 23, 2022 at 9:52 AM Yoav Weiss <[email protected]> >>>>>>> wrote: >>>>>>> >>>>>>>> As this is not actually a feature, but a spec alignment effort, I >>>>>>>> think it makes sense to skip TAG reviews and vendor positions for this. >>>>>>>> >>>>>>>> I'm slightly concerned about `Report` collisions. Would you be able >>>>>>>> to run a quick HTTPArchive scan for "window.Report" and "self.Report" >>>>>>>> and >>>>>>>> see what that gives? ("var Report" would just unconditionally override >>>>>>>> it, >>>>>>>> so that's not an issue) >>>>>>>> >>>>>>>> On Wed, Nov 23, 2022 at 2:55 PM Ian Clelland < >>>>>>>> [email protected]> wrote: >>>>>>>> >>>>>>>>> Contact [email protected] >>>>>>>>> >>>>>>>>> Specificationhttps://w3c.github.io/reporting/#idl-index >>>>>>>>> >>>>>>>>> Summary >>>>>>>>> >>>>>>>>> Several interface objects used by the Reporting API were >>>>>>>>> originally not exposed as symbols on the JavaScript global object. >>>>>>>>> This >>>>>>>>> includes Report, ReportBody, and the various specific report body >>>>>>>>> types >>>>>>>>> such as CSPViolationReportBody and DeprecationReportBody. This change >>>>>>>>> exposes those objects on the window (or worker global), bringing the >>>>>>>>> implementation in line with the specification. >>>>>>>>> >>>>>>>>> >>>>>>>>> Blink componentBlink>ReportingObserver >>>>>>>>> <https://bugs.chromium.org/p/chromium/issues/list?q=component:Blink%3EReportingObserver> >>>>>>>>> >>>>>>>>> TAG review >>>>>>>>> >>>>>>>>> TAG review statusNot applicable >>>>>>>>> >>>>>>>>> Risks >>>>>>>>> >>>>>>>>> >>>>>>>>> Interoperability and Compatibility >>>>>>>>> >>>>>>>>> The main risk here is that the newly-exposed symbols will collide >>>>>>>>> with symbols defined in existing scripts. Most of the names are quite >>>>>>>>> specific, but "Report" and "ReportBody" may have some existing use. >>>>>>>>> However, this is unlikely to be a problem in practice, as most code >>>>>>>>> which >>>>>>>>> uses these names for its own variables will also continue to work; >>>>>>>>> redefining them in a local script should not have any effect on >>>>>>>>> either the >>>>>>>>> script or the Reporting API. One possibility for breakage would be a >>>>>>>>> script >>>>>>>>> that defines, say, a "Report" object, (unrelated to the Reporting >>>>>>>>> API,) but >>>>>>>>> does so only if it did not previously exist. In that case, the >>>>>>>>> hypothetical >>>>>>>>> script would *not* define its own Report, and would presumably >>>>>>>>> attempt to >>>>>>>>> use the newly-exposed interface object. In order for this to be a real >>>>>>>>> issue, a script would have to: 1. Use one of these symbol names, down >>>>>>>>> to >>>>>>>>> capitalization. I suspect that "Report" is the most probable, with the >>>>>>>>> various ReportBody types being far less likely candidates for >>>>>>>>> collision 2. >>>>>>>>> Specifically guard against re-defining the symbol, even though it's >>>>>>>>> not >>>>>>>>> currently platform-exposed anywhere. This would likely only occur if >>>>>>>>> some >>>>>>>>> initialization code was expected to be run multiple times blindly. >>>>>>>>> (Blink's >>>>>>>>> web test result page, for instance, uses "Report" as an object name, >>>>>>>>> but >>>>>>>>> defines it unconditionally, so it will simply shadow the interface >>>>>>>>> object.) >>>>>>>>> >>>>>>>>> >>>>>>>>> *Gecko*: No signal >>>>>>>>> >>>>>>>>> *WebKit*: No signal >>>>>>>>> >>>>>>>>> *Web developers*: No signals >>>>>>>>> >>>>>>>>> *Other signals*: >>>>>>>>> >>>>>>>>> Ergonomics >>>>>>>>> >>>>>>>>> None >>>>>>>>> >>>>>>>>> >>>>>>>>> Activation >>>>>>>>> >>>>>>>>> None >>>>>>>>> >>>>>>>>> >>>>>>>>> Security >>>>>>>>> >>>>>>>>> None >>>>>>>>> >>>>>>>>> >>>>>>>>> WebView application risks >>>>>>>>> >>>>>>>>> Does this intent deprecate or change behavior of existing APIs, >>>>>>>>> such that it has potentially high risk for Android WebView-based >>>>>>>>> applications? >>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> Debuggability >>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> Will this feature be supported on all six Blink platforms >>>>>>>>> (Windows, Mac, Linux, Chrome OS, Android, and Android WebView)?Yes >>>>>>>>> >>>>>>>>> This is a change to the IDL for these interfaces, and is >>>>>>>>> automatically supported by all Blink platforms. >>>>>>>>> >>>>>>>>> >>>>>>>>> Is this feature fully tested by web-platform-tests >>>>>>>>> <https://chromium.googlesource.com/chromium/src/+/main/docs/testing/web_platform_tests.md> >>>>>>>>> ?Yes >>>>>>>>> >>>>>>>>> Flag name >>>>>>>>> >>>>>>>>> Requires code in //chrome?False >>>>>>>>> >>>>>>>>> Tracking bug >>>>>>>>> https://bugs.chromium.org/p/chromium/issues/detail?id=1122656 >>>>>>>>> >>>>>>>>> Estimated milestones >>>>>>>>> >>>>>>>>> M110 >>>>>>>>> >>>>>>>>> Anticipated spec changes >>>>>>>>> >>>>>>>>> Open questions about a feature may be a source of future web >>>>>>>>> compat or interop issues. Please list open issues (e.g. links to known >>>>>>>>> github issues in the project for the feature specification) whose >>>>>>>>> resolution may introduce web compat/interop risk (e.g., changing to >>>>>>>>> naming >>>>>>>>> or structure of the API in a non-backward-compatible way). >>>>>>>>> >>>>>>>>> >>>>>>>>> Link to entry on the Chrome Platform Status >>>>>>>>> https://chromestatus.com/feature/5977883141472256 >>>>>>>>> >>>>>>>>> This intent message was generated by Chrome Platform Status >>>>>>>>> <https://chromestatus.com/>. >>>>>>>>> >>>>>>>>> -- >>>>>>>>> You received this message because you are subscribed to the Google >>>>>>>>> Groups "blink-dev" group. >>>>>>>>> To unsubscribe from this group and stop receiving emails from it, >>>>>>>>> send an email to [email protected]. >>>>>>>>> To view this discussion on the web visit >>>>>>>>> https://groups.google.com/a/chromium.org/d/msgid/blink-dev/CAK_TSXKUf6wAHB-3EdgP04%2Bcmhgis3j3M54B%3DASXGtHVJcenDQ%40mail.gmail.com >>>>>>>>> <https://groups.google.com/a/chromium.org/d/msgid/blink-dev/CAK_TSXKUf6wAHB-3EdgP04%2Bcmhgis3j3M54B%3DASXGtHVJcenDQ%40mail.gmail.com?utm_medium=email&utm_source=footer> >>>>>>>>> . >>>>>>>>> >>>>>>>> -- >>>>>>>> You received this message because you are subscribed to the Google >>>>>>>> Groups "blink-dev" group. >>>>>>>> To unsubscribe from this group and stop receiving emails from it, >>>>>>>> send an email to [email protected]. >>>>>>>> To view this discussion on the web visit >>>>>>>> https://groups.google.com/a/chromium.org/d/msgid/blink-dev/CAL5BFfV3b3BnY26B6tuGtFFYRm-K5P0U6b5_giLgB1h6PeK7ag%40mail.gmail.com >>>>>>>> <https://groups.google.com/a/chromium.org/d/msgid/blink-dev/CAL5BFfV3b3BnY26B6tuGtFFYRm-K5P0U6b5_giLgB1h6PeK7ag%40mail.gmail.com?utm_medium=email&utm_source=footer> >>>>>>>> . >>>>>>>> >>>>>>> -- >> You received this message because you are subscribed to the Google Groups >> "blink-dev" group. >> To unsubscribe from this group and stop receiving emails from it, send an >> email to [email protected]. >> To view this discussion on the web visit >> https://groups.google.com/a/chromium.org/d/msgid/blink-dev/CAFUtAY8pXY9nUDxRwXTheVUCf0A81cZyfJLE7WDA%2B4%3Dqf-DiJQ%40mail.gmail.com >> <https://groups.google.com/a/chromium.org/d/msgid/blink-dev/CAFUtAY8pXY9nUDxRwXTheVUCf0A81cZyfJLE7WDA%2B4%3Dqf-DiJQ%40mail.gmail.com?utm_medium=email&utm_source=footer> >> . >> > -- You received this message because you are subscribed to the Google Groups "blink-dev" group. To unsubscribe from this group and stop receiving emails from it, send an email to [email protected]. To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/blink-dev/CAL5BFfW6ozjzdrHX9X9j%2BEKdEJ_OWa%2BtzT0A1%3DV_HRMnVwWxxg%40mail.gmail.com.
