Bug 183650

Created attachment 335830 [details] Patch

Created attachment 335831 [details] [Image] After Patch is applied

Created attachment 335836 [details] Patch Oops. Forgot some other inspector tests.

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>