Bug 53299

Created attachment 80444 [details] Patch

Comment on attachment 80444 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=80444&action=review This change needs further splitting. Otherwise, it is hard to follow. > Source/WebCore/inspector/front-end/Settings.js:34 > + sourceEditorEnabled: false, Is this change relevant? > Source/WebCore/inspector/front-end/SourceFrame.js:-212 > - if (this._canEditScripts) Where did this go? > Source/WebCore/inspector/front-end/SourceFrame.js:853 > + if (!Preferences.canEditScriptSource || !this._isScript || Preferences.sourceEditorEnabled) I think it is too early to introduce sourceEditorEnable switch. > Source/WebCore/inspector/front-end/TextViewer.js:-44 > - this.element.addEventListener("beforecopy", this._beforeCopy.bind(this), false); Where did these go? > Source/WebCore/inspector/front-end/TextViewer.js:46 > + var mainElm = this._mainPanel.element; Do not abbreviate ("mainElement") > Source/WebCore/inspector/front-end/TextViewer.js:47 > + mainElm.addEventListener("scroll", this._syncScroll.bind(this), false); It would be nice to encapsulate these since they serve same goal. Also, you might want to bind once. > Source/WebCore/inspector/front-end/TextViewer.js:52 > + // FIXME: Remove old live editing functionality and Preferences.sourceEditorEnabled flag. ditto (too early) > Source/WebCore/inspector/front-end/TextViewer.js:75 > + this._mainPanel.addDecoration(lineNumber, decoration); Why is this code getting more complex? > Source/WebCore/inspector/front-end/TextViewer.js:84 > + this._mainPanel.removeDecoration(lineNumber, decoration); ditto > Source/WebCore/inspector/front-end/TextViewer.js:112 > + _handleKeyDown: function() Did this method change? It is nearly impossible to track changes in this diff. Can we please split it into a series of refactorings? > Source/WebCore/inspector/front-end/TextViewer.js:236 > +//================================================================================================== No comments like this in WebKit please. > Source/WebCore/inspector/front-end/TextViewer.js:237 > +// WebInspector.TextEditorChunkedPanel ditto > Source/WebCore/inspector/front-end/TextViewer.js:421 > +//================================================================================================== ditto > Source/WebCore/inspector/front-end/TextViewer.js:458 > +//================================================================================================== ditto > Source/WebCore/inspector/front-end/TextViewer.js:-341 > - var offset = 0; Why did this change?

Created attachment 80734 [details] Patch

Comment on attachment 80444 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=80444&action=review >> Source/WebCore/inspector/front-end/Settings.js:34 >> + sourceEditorEnabled: false, > > Is this change relevant? I removed this flag for now >> Source/WebCore/inspector/front-end/SourceFrame.js:-212 >> - if (this._canEditScripts) > > Where did this go? This is a dead code, I just could not pass by :) revert it? >> Source/WebCore/inspector/front-end/SourceFrame.js:853 >> + if (!Preferences.canEditScriptSource || !this._isScript || Preferences.sourceEditorEnabled) > > I think it is too early to introduce sourceEditorEnable switch. reverted >> Source/WebCore/inspector/front-end/TextViewer.js:-44 >> - this.element.addEventListener("beforecopy", this._beforeCopy.bind(this), false); > > Where did these go? We do not have to override beforecopy/copy now - the native copy works just fine. It did not before, because we had TABLE and text selection would include line numbers as well. The only thing is decorations: they also will be included in the text selection, if they are selected (although I use CSS style "-webkit-user-selection: none"). Is this OK? >> Source/WebCore/inspector/front-end/TextViewer.js:46 >> + var mainElm = this._mainPanel.element; > > Do not abbreviate ("mainElement") done >> Source/WebCore/inspector/front-end/TextViewer.js:47 >> + mainElm.addEventListener("scroll", this._syncScroll.bind(this), false); > > It would be nice to encapsulate these since they serve same goal. Also, you might want to bind once. done >> Source/WebCore/inspector/front-end/TextViewer.js:52 >> + // FIXME: Remove old live editing functionality and Preferences.sourceEditorEnabled flag. > > ditto (too early) done >> Source/WebCore/inspector/front-end/TextViewer.js:75 >> + this._mainPanel.addDecoration(lineNumber, decoration); > > Why is this code getting more complex? Because, for example, we want to add the "webkit-execution-line" class name to both gutter and main panel lines/chunks. >> Source/WebCore/inspector/front-end/TextViewer.js:112 >> + _handleKeyDown: function() > > Did this method change? It is nearly impossible to track changes in this diff. Can we please split it into a series of refactorings? this particular method did not change >> Source/WebCore/inspector/front-end/TextViewer.js:236 >> +//================================================================================================== > > No comments like this in WebKit please. done >> Source/WebCore/inspector/front-end/TextViewer.js:237 >> +// WebInspector.TextEditorChunkedPanel > > ditto done >> Source/WebCore/inspector/front-end/TextViewer.js:-341 >> - var offset = 0; > > Why did this change? This was changed only slightly and still preserved in the _repaintAll method (see above).

Created attachment 80754 [details] Patch

Comment on attachment 80754 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=80754&action=review Few minor nits, otherwise looks good. > Source/WebCore/inspector/front-end/TextViewer.js:41 > this.element.addEventListener("keydown", this._handleKeyDown.bind(this), false); Why is this registered not in the this._mainPanel? > Source/WebCore/inspector/front-end/TextViewer.js:48 > + this._mainPanel.syncScrollListener = this._syncScroll.bind(this); Can we pass these as constructor parameters instead? (they are not re-assignable, right?) > Source/WebCore/inspector/front-end/TextViewer.js:166 > + if (lineNumber >= this._textModel.linesCount) Can this actually happen? > Source/WebCore/inspector/front-end/TextViewer.js:572 > + this.element.addEventListener("DOMCharacterDataModified", this._handleDomUpdates.bind(this), false); should be handleDOMUpdates. Again, do you want to reuse the bound instance? > Source/WebCore/inspector/front-end/TextViewer.js:898 > + this._syncDecorationsForLineListener(lineRow.lineNumber); This does not look robust. Are you sure you need it? > Source/WebCore/inspector/front-end/TextViewer.js:930 > + this.element.appendChild(document.createElement("br")); Will this affect drag'n'drop / copy?

Comment on attachment 80754 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=80754&action=review >> Source/WebCore/inspector/front-end/TextViewer.js:41 >> this.element.addEventListener("keydown", this._handleKeyDown.bind(this), false); > > Why is this registered not in the this._mainPanel? done. also, seems like this event can be dropped out as well (not doing this in the CL) >> Source/WebCore/inspector/front-end/TextViewer.js:48 >> + this._mainPanel.syncScrollListener = this._syncScroll.bind(this); > > Can we pass these as constructor parameters instead? (they are not re-assignable, right?) done >> Source/WebCore/inspector/front-end/TextViewer.js:166 >> + if (lineNumber >= this._textModel.linesCount) > > Can this actually happen? theoretically yes, provided that this method is called asynchronously >> Source/WebCore/inspector/front-end/TextViewer.js:572 >> + this.element.addEventListener("DOMCharacterDataModified", this._handleDomUpdates.bind(this), false); > > should be handleDOMUpdates. Again, do you want to reuse the bound instance? http://webkit.org/coding/coding-style.html: Lower-case the first letter, including all letters in an acronym, in a variable or function name. reused the bound instance. >> Source/WebCore/inspector/front-end/TextViewer.js:898 >> + this._syncDecorationsForLineListener(lineRow.lineNumber); > > This does not look robust. Are you sure you need it? Yes. I added the following comment: // Wait until this event is processed and only then sync the sizes. This is necessary in // case of the DOMNodeRemoved event, because it is dispatched before the removal takes place. (see http://www.w3.org/TR/DOM-Level-3-Events/#event-type-DOMNodeRemoved) >> Source/WebCore/inspector/front-end/TextViewer.js:930 >> + this.element.appendChild(document.createElement("br")); > > Will this affect drag'n'drop / copy? Nope. In fact, this is exactly what the browser would do, if you make an empty line in a content editable DIVs.

Created attachment 80907 [details] Patch

Attachment 80907 [details] did not pass style-queue: Failed to run "['Tools/Scripts/update-webkit']" exit_code: 2 perl: warning: Setting locale failed. perl: warning: Please check that your locale settings: LANGUAGE = (unset), LC_ALL = (unset), LANG = "en_US.US-ASCII" are supported and installed on your system. perl: warning: Falling back to the standard locale ("C"). Updating OpenSource perl: warning: Setting locale failed. perl: warning: Please check that your locale settings: LANGUAGE = (unset), LC_ALL = (unset), LANG = "en_US.US-ASCII" are supported and installed on your system. perl: warning: Falling back to the standard locale ("C"). RA layer request failed: OPTIONS of 'http://svn.webkit.org/repository/webkit': could not connect to server (http://svn.webkit.org) at /usr/lib/git-core/git-svn line 2295 Died at Tools/Scripts/update-webkit line 132. If any of these errors are false positives, please file a bug against check-webkit-style.

>>>> should be handleDOMUpdates. Again, do you want to reuse the bound instance? >> http://webkit.org/coding/coding-style.html: Lower-case the first letter, >> including all letters in an acronym, in a variable or function name. should be: handleDOMUpdates or domUpdatesHandler I think your quote refers to the second case.

(In reply to comment #10) > >>>> should be handleDOMUpdates. Again, do you want to reuse the bound instance? > > >> http://webkit.org/coding/coding-style.html: Lower-case the first letter, > >> including all letters in an acronym, in a variable or function name. > > should be: > handleDOMUpdates or > domUpdatesHandler > > I think your quote refers to the second case. done

Created attachment 80911 [details] Patch

Attachment 80911 [details] did not pass style-queue: Failed to run "['Tools/Scripts/update-webkit']" exit_code: 2 perl: warning: Setting locale failed. perl: warning: Please check that your locale settings: LANGUAGE = (unset), LC_ALL = (unset), LANG = "en_US.US-ASCII" are supported and installed on your system. perl: warning: Falling back to the standard locale ("C"). Updating OpenSource perl: warning: Setting locale failed. perl: warning: Please check that your locale settings: LANGUAGE = (unset), LC_ALL = (unset), LANG = "en_US.US-ASCII" are supported and installed on your system. perl: warning: Falling back to the standard locale ("C"). RA layer request failed: OPTIONS of 'http://svn.webkit.org/repository/webkit': could not connect to server (http://svn.webkit.org) at /usr/lib/git-core/git-svn line 2295 Died at Tools/Scripts/update-webkit line 132. If any of these errors are false positives, please file a bug against check-webkit-style.

Committed r77375: <http://trac.webkit.org/changeset/77375>