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/CAOMQ%2Bw_OVhUp1CuMnqywfjWpiSwHBXW0YpJbKC5hRMjT0-kJaw%40mail.gmail.com.
