Summary: | Web Inspector: Graphics: add instrumentation for new `CanvasRenderingContext2DSettings` | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Devin Rousso <hi> | ||||||||
Component: | Web Inspector | Assignee: | Devin Rousso <hi> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | bburg, ews-watchlist, hi, inspector-bugzilla-changes, joepeck, keith_miller, mark.lam, msaboff, pangle, saam, timothy, tzagallo, webkit-bug-importer | ||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||
Version: | WebKit Nightly Build | ||||||||||
Hardware: | All | ||||||||||
OS: | All | ||||||||||
Bug Depends on: | 225173 | ||||||||||
Bug Blocks: | |||||||||||
Attachments: |
|
Description
Devin Rousso
2021-04-28 19:07:38 PDT
Created attachment 435467 [details]
Patch
This patch modifies the inspector protocol. Please ensure that any frontend changes appropriately use feature checks for new protocol features. Created attachment 435499 [details]
Patch
Comment on attachment 435499 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=435499&action=review Looks good, just a few nits, and maybe some tests to reenable in another bug. Provisional r+ from me. > Source/WebCore/ChangeLog:14 > + Unify the two cases where we fetch canvas attributes into a single method so that both get > + the same data. This means that the canvas recorder now also uses `Canvas.ContextAttributes`. Nice. > Source/WebCore/inspector/InspectorCanvas.cpp:831 > +#endif NIT: `#endif // ENABLE(DESTINATION_COLOR_SPACE_DISPLAY_P3)` > Source/WebCore/inspector/InspectorCanvas.cpp:860 > + contextAttributesPayload->setPowerPreference("default"_s); Maybe power preference values should be an enum in the Protocol? > Source/WebCore/inspector/InspectorCanvas.cpp:863 > + contextAttributesPayload->setPowerPreference("low-power"_s); Ditto :860 > Source/WebCore/inspector/InspectorCanvas.cpp:866 > + contextAttributesPayload->setPowerPreference("high-performance"_s); Ditto :860 > LayoutTests/ChangeLog:24 > + * inspector/canvas/context-attributes-expected.txt: > + * inspector/canvas/recording-2d-frameCount-expected.txt: > + * inspector/canvas/recording-2d-full-expected.txt: > + * inspector/canvas/recording-2d-memoryLimit-expected.txt: > + * inspector/canvas/recording-bitmaprenderer-frameCount-expected.txt: > + * inspector/canvas/recording-bitmaprenderer-full-expected.txt: > + * inspector/canvas/recording-bitmaprenderer-memoryLimit-expected.txt: > + * inspector/canvas/recording-html-2d-expected.txt: > + * inspector/canvas/recording-webgl-frameCount-expected.txt: > + * inspector/canvas/recording-webgl-full-expected.txt: > + * inspector/canvas/recording-webgl-memoryLimit-expected.txt: > + * inspector/canvas/recording-webgl-snapshots-expected.txt: > + * inspector/canvas/recording-webgl2-frameCount-expected.txt: > + * inspector/canvas/recording-webgl2-full-expected.txt: > + * inspector/canvas/recording-webgl2-memoryLimit-expected.txt: > + * inspector/canvas/recording-webgl2-snapshots-expected.txt: Looking at the `Mac` `TestExpectations` file shows that some of these tests are `[Pass Timeout]` or `[Pass Failure Timeout]`. If any of these shouldn't be any more, we should probably update the expectations in another bug. Particularly the timeouts given that any modified test or test expectations get stress-tested in EWS now. Comment on attachment 435499 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=435499&action=review >> Source/WebCore/inspector/InspectorCanvas.cpp:860 >> + contextAttributesPayload->setPowerPreference("default"_s); > > Maybe power preference values should be an enum in the Protocol? Hmm, we could? We don't actually explicitly use these values manually anywhere in the frontend. It'd just be a sanity check on the backend really. The frontend pipes this data directly into a `canvasElement.createContext(type, attributes)`. Frankly we could replace `Canvas.ContextAttributes` with an anonymous object and it'd work the same. Comment on attachment 435499 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=435499&action=review r=me The Windows EWS failure looks like a clean build is needed, for some reason the InspectorProtocolObjects.h file is not being regenerated. Please confirm before landing. https://ews-build.webkit.org/#/builders/10/builds/102675 >>> Source/WebCore/inspector/InspectorCanvas.cpp:860 >>> + contextAttributesPayload->setPowerPreference("default"_s); >> >> Maybe power preference values should be an enum in the Protocol? > > Hmm, we could? We don't actually explicitly use these values manually anywhere in the frontend. It'd just be a sanity check on the backend really. The frontend pipes this data directly into a `canvasElement.createContext(type, attributes)`. Frankly we could replace `Canvas.ContextAttributes` with an anonymous object and it'd work the same. I'm with Devin here. I don't see much point in making an enum if we aren't verifying the values that come through. Comment on attachment 435499 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=435499&action=review >> Source/WebCore/inspector/InspectorCanvas.cpp:831 >> +#endif > > NIT: `#endif // ENABLE(DESTINATION_COLOR_SPACE_DISPLAY_P3)` While I personally would prefer doing this, I've been told in the past that WebKit style is (usually) to not include this unless there's a large gap between the `#if` and `#endif`. >> LayoutTests/ChangeLog:24 >> + * inspector/canvas/recording-webgl2-snapshots-expected.txt: > > Looking at the `Mac` `TestExpectations` file shows that some of these tests are `[Pass Timeout]` or `[Pass Failure Timeout]`. If any of these shouldn't be any more, we should probably update the expectations in another bug. Particularly the timeouts given that any modified test or test expectations get stress-tested in EWS now. I think this could be said for _all_ Web Inspector tests :P Definitely worth looking at this, but I'd likely do that in a separate bug so as to not cause additional issues. Patch 435499 does not build Created attachment 439015 [details]
Patch
Committed r282984 (242069@main): <https://commits.webkit.org/242069@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 439015 [details]. |