WebKit Bugzilla
Attachment 358205 Details for
Bug 192739
: Web Inspector: Styles: fix race conditions when editing
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Requests
|
Help
|
New Account
|
Log In
Remember
[x]
|
Forgot Password
Login:
[x]
[patch]
Patch
patch.txt (text/plain), 7.66 KB, created by
Nikita Vasilyev
on 2019-01-02 14:18:03 PST
(
hide
)
Description:
Patch
Filename:
MIME Type:
Creator:
Nikita Vasilyev
Created:
2019-01-02 14:18:03 PST
Size:
7.66 KB
patch
obsolete
>diff --git a/Source/WebInspectorUI/ChangeLog b/Source/WebInspectorUI/ChangeLog >index d0d8c6c2cb9..e194b71639c 100644 >--- a/Source/WebInspectorUI/ChangeLog >+++ b/Source/WebInspectorUI/ChangeLog >@@ -1,3 +1,40 @@ >+2019-01-02 Nikita Vasilyev <nvasilyev@apple.com> >+ >+ Web Inspector: Styles: fix race conditions when editing >+ https://bugs.webkit.org/show_bug.cgi?id=192739 >+ <rdar://problem/46752925> >+ >+ Reviewed by NOBODY (OOPS!). >+ >+ Style updates are asynchronous. For example, there are two updates, A and B: >+ >+ A: [------] >+ B: [--] >+ >+ B may finish before A, even though B started after A. >+ >+ Unsure there's no race conditions by introducing `_updatesInProgressCount`: >+ >+ - Increment it before calling CSSAgent.setStyleText. >+ - Decrement it after CSSAgent.setStyleText is finished. >+ >+ Prevent updates of CSSStyleDeclaration when _updatesInProgressCount isn't 0. >+ >+ * UserInterface/Models/CSSProperty.js: >+ (WI.CSSProperty.prototype._updateOwnerStyleText): >+ * UserInterface/Models/CSSStyleDeclaration.js: >+ (WI.CSSStyleDeclaration): >+ (WI.CSSStyleDeclaration.prototype.set text): Removed. >+ (WI.CSSStyleDeclaration.prototype.setText): Added. >+ Change the setter to a method since it has side effects including an asynchronous backend call. >+ >+ * UserInterface/Models/DOMNodeStyles.js: >+ (WI.DOMNodeStyles.prototype.changeStyleText): >+ >+ * UserInterface/Views/SpreadsheetStyleProperty.js: >+ (WI.SpreadsheetStyleProperty.prototype.get property): >+ Drive-by: remove unused code. >+ > 2018-12-21 Devin Rousso <drousso@apple.com> > > Web Inspector: Styles Redesign: remove unused CSS style icons >diff --git a/Source/WebInspectorUI/UserInterface/Models/CSSProperty.js b/Source/WebInspectorUI/UserInterface/Models/CSSProperty.js >index 4ba36efc8c3..b61f3d77a23 100644 >--- a/Source/WebInspectorUI/UserInterface/Models/CSSProperty.js >+++ b/Source/WebInspectorUI/UserInterface/Models/CSSProperty.js >@@ -383,7 +383,8 @@ WI.CSSProperty = class CSSProperty extends WI.Object > let columnDelta = newText.lastLine.length - oldText.lastLine.length; > this._styleSheetTextRange = this._styleSheetTextRange.cloneAndModify(0, 0, lineDelta, columnDelta); > >- this._ownerStyle.text = newStyleText; >+ // This causes a backend call which is asynchronous yet changes in the model on the front-end are synchronous. >+ this._ownerStyle.setText(newStyleText); > > let propertyWasRemoved = !newText; > this._ownerStyle.shiftPropertiesAfter(this, lineDelta, columnDelta, propertyWasRemoved); >diff --git a/Source/WebInspectorUI/UserInterface/Models/CSSStyleDeclaration.js b/Source/WebInspectorUI/UserInterface/Models/CSSStyleDeclaration.js >index 62f11405844..da3d05930b3 100644 >--- a/Source/WebInspectorUI/UserInterface/Models/CSSStyleDeclaration.js >+++ b/Source/WebInspectorUI/UserInterface/Models/CSSStyleDeclaration.js >@@ -40,6 +40,7 @@ WI.CSSStyleDeclaration = class CSSStyleDeclaration extends WI.Object > this._node = node || null; > this._inherited = inherited || false; > >+ this._updatesInProgressCount = 0; > this._locked = false; > this._pendingProperties = []; > this._propertyNameMap = {}; >@@ -102,9 +103,9 @@ WI.CSSStyleDeclaration = class CSSStyleDeclaration extends WI.Object > update(text, properties, styleSheetTextRange, options = {}) > { > let dontFireEvents = options.dontFireEvents || false; >- let suppressLock = options.suppressLock || false; >+ let isLocked = !options.suppressLock || this._locked; > >- if (this._locked && !suppressLock && text !== this._text) >+ if (isLocked && this._updatesInProgressCount > 0 && text !== this._text) > return; > > text = text || ""; >@@ -168,7 +169,7 @@ WI.CSSStyleDeclaration = class CSSStyleDeclaration extends WI.Object > > // Don't fire the event if there is text and it hasn't changed. > if (oldText && this._text && oldText === this._text) { >- if (!this._locked || suppressLock) { >+ if (!isLocked) { > // We shouldn't have any added or removed properties in this case. > console.assert(!addedProperties.length && !removedProperties.length); > } >@@ -201,7 +202,7 @@ WI.CSSStyleDeclaration = class CSSStyleDeclaration extends WI.Object > return this._text; > } > >- set text(text) >+ setText(text) > { > if (this._text === text) > return; >@@ -213,11 +214,30 @@ WI.CSSStyleDeclaration = class CSSStyleDeclaration extends WI.Object > if (!trimmedText.length || this._type === WI.CSSStyleDeclaration.Type.Inline) > text = trimmedText; > >- // Update text immediately when it was modified via the styles sidebar. >- if (this._locked) >- this._text = text; >+ this._text = text; >+ >+ // Updates are asynchronous. >+ // A: [------] >+ // B: [--] >+ // B may finish before A, even though B started after A. >+ this._updatesInProgressCount++; >+ >+ let styleTextDidChange = () => { >+ if (!timeoutId) >+ return; >+ >+ clearTimeout(timeoutId); >+ timeoutId = null; >+ this._updatesInProgressCount--; >+ console.assert(this._updatesInProgressCount >= 0, "_updatesInProgressCount cannot be a negative number."); >+ }; >+ >+ let timeoutId = setTimeout(() => { >+ console.error("Timed out when setting style text:", text); >+ styleTextDidChange(); >+ }, 1000); > >- this._nodeStyles.changeStyleText(this, text); >+ this._nodeStyles.changeStyleText(this, text, styleTextDidChange); > } > > get properties() >diff --git a/Source/WebInspectorUI/UserInterface/Models/DOMNodeStyles.js b/Source/WebInspectorUI/UserInterface/Models/DOMNodeStyles.js >index f44b05628a1..32a8b939ba2 100644 >--- a/Source/WebInspectorUI/UserInterface/Models/DOMNodeStyles.js >+++ b/Source/WebInspectorUI/UserInterface/Models/DOMNodeStyles.js >@@ -449,21 +449,26 @@ WI.DOMNodeStyles = class DOMNodeStyles extends WI.Object > return result.promise; > } > >- changeStyleText(style, text) >+ changeStyleText(style, text, callback = null) > { > if (!style.ownerStyleSheet || !style.styleSheetTextRange) > return; > > text = text || ""; > >- function styleChanged(error, stylePayload) >- { >- if (error) >+ let didSetStyleText = (error, stylePayload) => { >+ if (error) { >+ callback(error); > return; >- this.refresh(); >- } >+ } >+ >+ this.refresh().then(() => { >+ if (callback) >+ callback(); >+ }); >+ }; > >- CSSAgent.setStyleText(style.id, text, styleChanged.bind(this)); >+ CSSAgent.setStyleText(style.id, text, didSetStyleText); > } > > // Private >diff --git a/Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js b/Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js >index a41f95dfd64..bf3e43f9e0a 100644 >--- a/Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js >+++ b/Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js >@@ -71,8 +71,6 @@ WI.SpreadsheetStyleProperty = class SpreadsheetStyleProperty extends WI.Object > > get element() { return this._element; } > get property() { return this._property; } >- get nameTextField() { return this._nameTextField; } >- get valueTextField() { return this._valueTextField; } > get enabled() { return this._property.enabled; } > > set index(index)
You cannot view the attachment while viewing its details because your browser does not support IFRAMEs.
View the attachment on a separate page
.
View Attachment As Diff
View Attachment As Raw
Flags:
nvasilyev
:
review-
nvasilyev
:
commit-queue-
Actions:
View
|
Formatted Diff
|
Diff
Attachments on
bug 192739
:
358205
|
358212
|
358216
|
358217
|
358219
|
358447
|
358450
|
358451
|
358453
|
358466
|
358857
|
360990
|
360994
|
361001
|
361007
|
361008
|
361104