Instead of having a "View Recordings..." dropdown, we should just have two icons (one each for recordings and shaders) adopt a similar approach to path components (if there is only one, select it on click, otherwise show a dropdown). This way, all related "resources" to the canvas are easily accessible from the Overview.
Created attachment 335830 [details] Patch
Created attachment 335831 [details] [Image] After Patch is applied
Comment on attachment 335830 [details] Patch Attachment 335830 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/6961032 New failing tests: http/tests/inspector/network/resource-response-source-memory-cache.html http/tests/websocket/tests/hybi/inspector/before-load.html http/tests/inspector/network/resource-response-source-memory-cache-revalidate-expired-only.html http/tests/inspector/worker/blob-script-with-cross-domain-imported-scripts.html http/tests/inspector/network/set-resource-caching-disabled-memory-cache.html
Created attachment 335833 [details] Archive of layout-test-results from ews101 for mac-sierra The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews101 Port: mac-sierra Platform: Mac OS X 10.12.6
Comment on attachment 335830 [details] Patch Attachment 335830 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/6961053 New failing tests: http/tests/inspector/network/resource-response-source-memory-cache-revalidate-expired-only.html http/tests/inspector/network/set-resource-caching-disabled-memory-cache.html http/tests/inspector/network/resource-response-source-memory-cache.html http/tests/websocket/tests/hybi/inspector/before-load.html http/tests/inspector/network/har/har-page.html http/tests/inspector/worker/blob-script-with-cross-domain-imported-scripts.html
Created attachment 335834 [details] Archive of layout-test-results from ews104 for mac-sierra-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews104 Port: mac-sierra-wk2 Platform: Mac OS X 10.12.6
Comment on attachment 335830 [details] Patch Attachment 335830 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/6961062 New failing tests: http/tests/inspector/network/resource-response-source-memory-cache.html http/tests/websocket/tests/hybi/inspector/before-load.html http/tests/inspector/network/resource-response-source-memory-cache-revalidate-expired-only.html http/tests/inspector/worker/blob-script-with-cross-domain-imported-scripts.html http/tests/inspector/network/set-resource-caching-disabled-memory-cache.html
Created attachment 335835 [details] Archive of layout-test-results from ews115 for mac-sierra The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews115 Port: mac-sierra Platform: Mac OS X 10.12.6
Created attachment 335836 [details] Patch Oops. Forgot some other inspector tests.
Comment on attachment 335836 [details] Patch Attachment 335836 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/6961737 New failing tests: http/tests/inspector/network/har/har-page.html
Created attachment 335839 [details] Archive of layout-test-results from ews104 for mac-sierra-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews104 Port: mac-sierra-wk2 Platform: Mac OS X 10.12.6
Created attachment 335865 [details] Patch Splitting the changes to WI.Collection into a separate patch because of EWS failures
Comment on attachment 335865 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=335865&action=review I think this is a good idea. Often it can be useful to just see which context has shaders to know which is likely to be the important context. r- just so that we can see an updated patch. Otherwise, this is an r+ able patch. > Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js:1035 > -localizedStrings["View Recordings... (%d)"] = "View Recordings... (%d)"; > +localizedStrings["View Recording"] = "View Recording"; > +localizedStrings["View Shader"] = "View Shader"; Hmm, what if the UI looked like this: Shaders, No Recordings: [@] Shaders, 1 Recording: [@] o_o (1) Shaders, 2 Recordings: [@] o_o (2) I think the (#) gives extra information that wasn't available but might be useful to the user... "oh this is the one I took multiple recordings on". However if we think most users will only ever do a single recording then it probably won't Having a button to quickly get to the shaders seems nice. Seems like a good way to make use of an extra 20px that would otherwise not be used =) > Source/WebInspectorUI/UserInterface/Views/CanvasContentView.js:276 > + let {canvas} = event.data; Heh. I just ran some tests, and the perf of this is (as it should be) identical to the direct access. let {canvas} = event.data; let canvas = event.data.canvas; Seems like an interesting pattern that is a few less characters =) > Source/WebInspectorUI/UserInterface/Views/CanvasContentView.js:419 > + _updateViewRelated() I feel like this name needs another word. Maybe: _updateViewRelatedItems() > Source/WebInspectorUI/UserInterface/Views/CanvasContentView.js:440 > + if (shaderPrograms.length === 1) { > + WI.showRepresentedObject(shaderPrograms[0]); > + return; > + } I'd have to experience a picker. Is it worth having a picker or just getting the user to the UI and then letting them select the shader? > Source/WebInspectorUI/UserInterface/Views/CanvasContentView.js:463 > + if (recordings.length === 1) { > + WI.showRepresentedObject(recordings[0]); > + return; > + } Likewise. Just clicking the entire canvas gets the user into this. Is it worth having them select the recording here, or from the normal UI? I see these icons more as indicators than useful for actions.
Comment on attachment 335865 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=335865&action=review >> Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js:1035 >> +localizedStrings["View Shader"] = "View Shader"; > > Hmm, what if the UI looked like this: > > Shaders, No Recordings: > [@] > > Shaders, 1 Recording: > [@] o_o (1) > > Shaders, 2 Recordings: > [@] o_o (2) > > I think the (#) gives extra information that wasn't available but might be useful to the user... "oh this is the one I took multiple recordings on". However if we think most users will only ever do a single recording then it probably won't > > Having a button to quickly get to the shaders seems nice. Seems like a good way to make use of an extra 20px that would otherwise not be used =) My only hesitance to (#) is that it might be confusing as to which it relates to. I'd see a (#) and think "thats the total number of 'associated' things to this canvas (recordings + shaders)". Also, I don't think many WebGL canvases will have more than one or two shaders, so it's probably pretty rare. As a side note, I laughed at "o_o" 😛 >> Source/WebInspectorUI/UserInterface/Views/CanvasContentView.js:440 >> + } > > I'd have to experience a picker. Is it worth having a picker or just getting the user to the UI and then letting them select the shader? I think that any time we can eliminate an action by the user, it's a good idea. This follows the same pattern as path components, and I think we should do it. Also, see my above comment about the number of shaders. >> Source/WebInspectorUI/UserInterface/Views/CanvasContentView.js:463 >> + } > > Likewise. Just clicking the entire canvas gets the user into this. Is it worth having them select the recording here, or from the normal UI? > > I see these icons more as indicators than useful for actions. This actually shouldn't be the case. Recording's shouldn't be automatically selected when viewing a canvas unless the recording has just finished being captured.
Comment on attachment 335865 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=335865&action=review >>> Source/WebInspectorUI/UserInterface/Views/CanvasContentView.js:463 >>> + } >> >> Likewise. Just clicking the entire canvas gets the user into this. Is it worth having them select the recording here, or from the normal UI? >> >> I see these icons more as indicators than useful for actions. > > This actually shouldn't be the case. Recording's shouldn't be automatically selected when viewing a canvas unless the recording has just finished being captured. Are you referring to when we automatically show a recording once it completes? (which IIR is for any recording not started from the console). Clicking a canvas from the Overview should always show the same view, correct?
Comment on attachment 335865 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=335865&action=review >>>> Source/WebInspectorUI/UserInterface/Views/CanvasContentView.js:463 >>>> + } >>> >>> Likewise. Just clicking the entire canvas gets the user into this. Is it worth having them select the recording here, or from the normal UI? >>> >>> I see these icons more as indicators than useful for actions. >> >> This actually shouldn't be the case. Recording's shouldn't be automatically selected when viewing a canvas unless the recording has just finished being captured. > > Are you referring to when we automatically show a recording once it completes? (which IIR is for any recording not started from the console). Clicking a canvas from the Overview should always show the same view, correct? Yes. Clicking on a canvas in the overview should just show a `WI.CanvasContentView` (no recording selected) for that canvas. When a non-console recording finishes, a `WI.RecordingContentView` should be shown with similar sidebars as `WI.CanvasContentView` (any shaders are shown), except with recording specific data (e.g. actions list, other details sidebars, etc).
Created attachment 346364 [details] Patch This patch is mainly a rebase with a few style changes. My build is currently broken, so I was unable to test it. :/
Created attachment 347227 [details] Patch Fixed a few bugs
Comment on attachment 347227 [details] Patch r=me
Comment on attachment 347227 [details] Patch Clearing flags on attachment: 347227 Committed r235093: <https://trac.webkit.org/changeset/235093>
All reviewed patches have been landed. Closing bug.
<rdar://problem/43534413>