Bug 194318 - Web Inspector: Styles: PropertiesChanged shouldn't fire when old and new text are both empty
Summary: Web Inspector: Styles: PropertiesChanged shouldn't fire when old and new text...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Nikita Vasilyev
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-02-05 16:42 PST by Nikita Vasilyev
Modified: 2019-02-07 09:10 PST (History)
7 users (show)

See Also:


Attachments
Patch (6.57 KB, patch)
2019-02-05 16:49 PST, Nikita Vasilyev
drousso: review+
drousso: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews101 for mac-highsierra (2.56 MB, application/zip)
2019-02-05 18:09 PST, Build Bot
no flags Details
Patch (7.95 KB, patch)
2019-02-05 18:32 PST, Nikita Vasilyev
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Nikita Vasilyev 2019-02-05 16:42:46 PST
inspector/css/modify-css-property-race.html is still failing on debug build (and debug build only!) because of this.
https://webkit-queues.webkit.org/results/11045743

Also, it adds an unnecessary entropy to the universe :)
Comment 1 Nikita Vasilyev 2019-02-05 16:49:48 PST
Created attachment 361249 [details]
Patch
Comment 2 Devin Rousso 2019-02-05 17:05:36 PST
Comment on attachment 361249 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=361249&action=review

rs=me

> LayoutTests/inspector/css/modify-css-property-race.html:14
> +        let height = parseInt(element.style.height) + 1;

oops :P

> LayoutTests/inspector/css/modify-css-property-race.html:72
> +                    InspectorTest.expectGreaterThan(heightNumber, 11, "Height should be greater than 11px.");

Rather than compare the hardcoded 10 and 11, you could save a `previousHeightNumber` and ensure that the first one is greater than 10 and the second one is greater than `previousHeightNumber` (and change the text to say "Height should have increased from 10px" so you don't have to log the actual increased value).

> LayoutTests/inspector/css/modify-css-property-race.html:77
> +                    InspectorTest.expectGreaterThanOrEqual(heightNumber, 101, "Height should be greater or equal 101px.");

Ditto (72).
Comment 3 Build Bot 2019-02-05 18:09:40 PST
Comment on attachment 361249 [details]
Patch

Attachment 361249 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/11046952

New failing tests:
inspector/css/modify-css-property.html
Comment 4 Build Bot 2019-02-05 18:09:41 PST
Created attachment 361260 [details]
Archive of layout-test-results from ews101 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-highsierra  Platform: Mac OS X 10.13.6
Comment 5 Nikita Vasilyev 2019-02-05 18:19:20 PST
This broke other tests :(
Fix incoming.
Comment 6 Nikita Vasilyev 2019-02-05 18:32:16 PST
Created attachment 361262 [details]
Patch
Comment 7 Nikita Vasilyev 2019-02-05 18:43:48 PST
(In reply to Devin Rousso from comment #2)
> > LayoutTests/inspector/css/modify-css-property-race.html:72
> > +                    InspectorTest.expectGreaterThan(heightNumber, 11, "Height should be greater than 11px.");
> 
> Rather than compare the hardcoded 10 and 11, you could save a
> `previousHeightNumber` and ensure that the first one is greater than 10 and
> the second one is greater than `previousHeightNumber` (and change the text
> to say "Height should have increased from 10px" so you don't have to log the
> actual increased value).

I actually find it easier to follow when it's hardcoded there. However, I do like the less "robotic" assert messages!
Comment 8 WebKit Commit Bot 2019-02-05 22:54:48 PST
Comment on attachment 361262 [details]
Patch

Clearing flags on attachment: 361262

Committed r241011: <https://trac.webkit.org/changeset/241011>
Comment 9 WebKit Commit Bot 2019-02-05 22:54:49 PST
All reviewed patches have been landed.  Closing bug.
Comment 10 Radar WebKit Bug Importer 2019-02-05 22:55:30 PST
<rdar://problem/47842941>
Comment 11 Truitt Savell 2019-02-07 09:10:41 PST
After the fixes in https://trac.webkit.org/changeset/241011/webkit

inspector/css/modify-css-property-race.html

has become a flakey failure on Debug. Looks like it it somehow writing another pass message? 

History:
http://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=inspector%2Fcss%2Fmodify-css-property-race.html

Diff:
--- /Volumes/Data/slave/highsierra-debug-tests-wk1/build/layout-test-results/inspector/css/modify-css-property-race-expected.txt
+++ /Volumes/Data/slave/highsierra-debug-tests-wk1/build/layout-test-results/inspector/css/modify-css-property-race-actual.txt
@@ -7,4 +7,5 @@
 PASS: Height should have increased from 11px.
 PASS: Height should be 100px or more.
 PASS: Height should be 101px or more.
+PASS: Height should be 101px or more.