Bug 138593

<rdar://problem/18936194>

Created attachment 316816 [details] Patch

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

This will need to be rebased once https://bugs.webkit.org/show_bug.cgi?id=172624 lands.

Created attachment 317180 [details] Patch

Created attachment 317211 [details] Patch

Comment on attachment 317211 [details] Patch Attachment 317211 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/4250843 New failing tests: inspector/canvas/requestShaderSource.html

Created attachment 317214 [details] Archive of layout-test-results from ews116 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews116 Port: mac-elcapitan Platform: Mac OS X 10.11.6

Created attachment 317217 [details] Patch

Comment on attachment 317217 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=317217&action=review r=me, with comments. > Source/WebInspectorUI/ChangeLog:7 > + Reviewed by NOBODY (OOPS!). Please add a short summary of the frontend bits. > Source/WebCore/inspector/InspectorShaderProgram.cpp:69 > + return nullptr; This should never happen so we should assert: else { ASSERT_NOT_REACHED(); return nullptr; } > Source/WebInspectorUI/UserInterface/Models/ShaderProgram.js:58 > + } Style: newline after. > Source/WebInspectorUI/UserInterface/Views/ShaderProgramContentView.css:55 > + padding: 0.1em 0.5em; Nit: we usually use pixels for margin/padding. Our layout doesn't adapt to font size, and ems are convenient but more difficult to reason about. > Source/WebInspectorUI/UserInterface/Views/ShaderProgramContentView.css:61 > + top: 1.5em; See above. > Source/WebInspectorUI/UserInterface/Views/ShaderProgramContentView.js:48 > + Nit: remove space. > Source/WebInspectorUI/UserInterface/Views/ShaderProgramContentView.js:51 > + break; Nit: space after. > Source/WebInspectorUI/UserInterface/Views/ShaderProgramContentView.js:54 > + Ditto. > Source/WebInspectorUI/UserInterface/Views/ShaderProgramContentView.js:66 > + Remove space. > Source/WebInspectorUI/UserInterface/Views/ShaderProgramContentView.js:93 > + super.hidden(); In general calls to super should always be first, unless there is good reason. > Source/WebInspectorUI/UserInterface/Views/ShaderProgramContentView.js:101 > + super.closed(); Ditto. > Source/WebInspectorUI/UserInterface/Views/ShaderProgramContentView.js:124 > + get supportsSearch() Inline. > LayoutTests/inspector/canvas/requestShaderSource.html:61 > + // ------ Remove comment.

Comment on attachment 317217 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=317217&action=review >> Source/WebCore/inspector/InspectorShaderProgram.cpp:69 >> + return nullptr; > > This should never happen so we should assert: > > else { > ASSERT_NOT_REACHED(); > return nullptr; > } I didn't do this because then one of the tests I wrote fails. I don't think it's necessary to have this assert. It's already possible for this to return a nullptr if no shaders have been attached, so I think that callers will already have to check for nullptr. >> Source/WebInspectorUI/UserInterface/Views/ShaderProgramContentView.js:93 >> + super.hidden(); > > In general calls to super should always be first, unless there is good reason. I typically put hidden/closed super calls last in case they do something to the element that I don't expect. I follow a general->specific approach super.shown() this.shown() ... this.hidden() super.hidden() >> Source/WebInspectorUI/UserInterface/Views/ShaderProgramContentView.js:124 >> + get supportsSearch() > > Inline. I thought we only did this for obvious getters/setters, such as ones that assign/return a variable of the same name? I'm fine either way though. >> LayoutTests/inspector/canvas/requestShaderSource.html:61 >> + // ------ > > Remove comment. I've done this in most of my tests to distinguish that the tests below the line are for error cases.

Comment on attachment 317217 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=317217&action=review >>> Source/WebInspectorUI/UserInterface/Views/ShaderProgramContentView.js:124 >>> + get supportsSearch() >> >> Inline. > > I thought we only did this for obvious getters/setters, such as ones that assign/return a variable of the same name? I'm fine either way though. Our criteria in the past has been whether the body of the getter is trivial/does work. It is subjective, but usually we consider getters that don't call class methods or methods on members trivial. >>> LayoutTests/inspector/canvas/requestShaderSource.html:61 >>> + // ------ >> >> Remove comment. > > I've done this in most of my tests to distinguish that the tests below the line are for error cases. I don't think it is needed, but if you feel strongly about it at least mention that in the comment. As is, this looks like it was included by accident.

Created attachment 317265 [details] Patch

Comment on attachment 317265 [details] Patch Clearing flags on attachment: 317265 Committed r220294: <http://trac.webkit.org/changeset/220294>

All reviewed patches have been landed. Closing bug.

Comment on attachment 317265 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=317265&action=review Sweet! > Source/WebInspectorUI/Scripts/update-codemirror-resources.rb:44 > + mode/clike/clike.js Nice!! I didn't think anyone else knew about this script! =)

This looks just great. Thanks! Should it enabled by a build flag or something? I couldn't find the feature on the place depicted on the screenshot on Safari Technology Preview 45 on http://acko.net or https://ebraminio.github.io/extra/WebGL1.html so some help would be nice I guess

Guys, any pointer on how to see result of this? I still can't see result of this on "Release 47 (Safari 11.1, WebKit 13605.1.19.1)" maybe some build or runtime flag is needed?

(In reply to Ebrahim Byagowi from comment #18) > Guys, any pointer on how to see result of this? I still can't see result of > this on "Release 47 (Safari 11.1, WebKit 13605.1.19.1)" maybe some build or > runtime flag is needed? Viewable shader programs were broken but are being brought back in <https://webkit.org/b/178744>. This should be available in the next Safari Technology Preview (release 50).

Tested this yesterday with `brew cask reinstall webkit-build-archive`, works flawlessly, even doesn't need page reload and works great. Thanks!