This change is finalizing the migration to the experiment, fixing many things along the way.
Created attachment 133049 [details] Patch
Created attachment 133055 [details] Patch
Comment on attachment 133055 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=133055&action=review > Source/WebCore/inspector/front-end/JavaScriptSourceFrame.js:72 > + function mycallback(mimeType, content) myCallback? > Source/WebCore/inspector/front-end/JavaScriptSourceFrame.js:90 > editContent: function(newContent, callback) consider renaming to saveContent. > Source/WebCore/inspector/front-end/JavaScriptSourceFrame.js:92 > + this._originalContent = newContent; Please move to didEditContent > Source/WebCore/inspector/front-end/SourceFrame.js:97 > this._wasHiddenWhileEditing = true; Please remove these 2 lines. > Source/WebCore/inspector/front-end/SourceFrame.js:530 > doubleClick: function(lineNumber) Consider removing this method. > Source/WebCore/inspector/front-end/TextViewer.js:1016 > + this.revealLine(lineNumber); revealLine() should be called in both modes.
Please stop disguising random fixes under a bug that implies a simple switch is being flipped. Ideally these fixes would have been done in separate, logically grouped changes before the switch was flipped.
(In reply to comment #4) > Please stop disguising random fixes under a bug that implies a simple switch is being flipped. Ideally these fixes would have been done in separate, logically grouped changes before the switch was flipped. I am sorry, I don't get it. Could you be more specific?
The bug title implies this should be a simple 1-10 line change. Instead the change is a collection of misc fixes (not described in the ChangeLog either.) These fixes should be broken up into smaller patches, with descriptions of what was fixed in the ChangeLog for each function. Then a final change to move the feature out of experiment status. No comments in the ChangeLog is the main issue here. Breaking things up into smaller changes would be nice too.
(In reply to comment #6) > The bug title implies this should be a simple 1-10 line change. Instead the change is a collection of misc fixes (not described in the ChangeLog either.) > > These fixes should be broken up into smaller patches, with descriptions of what was fixed in the ChangeLog for each function. Then a final change to move the feature out of experiment status. > This change is not 'flipping the switch', all these changes are required for editing to be enabled by default. Changes under the experiment were only used to assess the user experience. I had no intention to complete it as an experiment and then flip the switch - too much work. > No comments in the ChangeLog is the main issue here. Breaking things up into smaller changes would be nice too. This change removes cancelEditing and setReadOnly. It establishes proper content dispatching. All three are inter-dependent, need to be landed simultaneously. Popover change removes the readOnly guard, also necessary. I'll update the log. Two drive-by fixes I can think of are: hiding popover upon Esc and a fix with the no-op change in the text model. I'll extract them into separate patches.
(In reply to comment #7) > (In reply to comment #6) > > The bug title implies this should be a simple 1-10 line change. Instead the change is a collection of misc fixes (not described in the ChangeLog either.) > > > > These fixes should be broken up into smaller patches, with descriptions of what was fixed in the ChangeLog for each function. Then a final change to move the feature out of experiment status. > > > > This change is not 'flipping the switch', all these changes are required for editing to be enabled by default. Changes under the experiment were only used to assess the user experience. I had no intention to complete it as an experiment and then flip the switch - too much work. Okay, the bug title (and lack of details) made it sound like a switch flip. > > No comments in the ChangeLog is the main issue here. Breaking things up into smaller changes would be nice too. > > This change removes cancelEditing and setReadOnly. It establishes proper content dispatching. All three are inter-dependent, need to be landed simultaneously. Popover change removes the readOnly guard, also necessary. I'll update the log. > > Two drive-by fixes I can think of are: hiding popover upon Esc and a fix with the no-op change in the text model. I'll extract them into separate patches. And I wouldn't know this without asking you or trying to infer this from the code. That is great info to add to the bug and the ChangeLog. Please revise this patch and add that info (ideally per changed function). I filed bug 81828 about making the style bot complain about sparse ChangeLogs.
Comment on attachment 133055 [details] Patch Please revise the ChangeLog.
(In reply to comment #9) > (From update of attachment 133055 [details]) > Please revise the ChangeLog. Now that you r- ed it, I hope you will be available to review revised ChangeLog tomorrow at 4am PST :)
(In reply to comment #10) > (In reply to comment #9) > > (From update of attachment 133055 [details] [details]) > > Please revise the ChangeLog. > > Now that you r- ed it, I hope you will be available to review revised ChangeLog tomorrow at 4am PST :) It is the responsibility of all reviewers to make sure the ChangeLog has decent info. If not, maybe they shouldn't be reviewers. http://www.webkit.org/coding/contributing.html#changelogs
Created attachment 133241 [details] Patch
Comment on attachment 133241 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=133241&action=review > Source/WebCore/ChangeLog:9 > + I removes dblclick handler as well since one does not need to enter editing mode. I -> It > Source/WebCore/inspector/front-end/TextViewer.js:1173 > + this._restoreSelection(newRange.collapseToEnd(), true); Please mention this change in ChangeLog
Committed r111682: <http://trac.webkit.org/changeset/111682>