Bug 176279 – Fix a few minor problems found while working toward removing unneeded calls to updateStyle

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Darin Adler]

Fix a few minor problems found while working toward removing unneeded calls to updateStyle

[Darin Adler]

Created attachment 319725 [details]
Patch

[Build Bot]

Comment on attachment 319725 [details]
Patch

Attachment 319725 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/4436233

New failing tests:
imported/w3c/web-platform-tests/html/dom/documents/dom-tree-accessors/document.title-09.html

[Build Bot]

Created attachment 319734 [details]
Archive of layout-test-results from ews100 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 319725 [details]
Patch

Attachment 319725 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/4436236

New failing tests:
imported/w3c/web-platform-tests/html/dom/documents/dom-tree-accessors/document.title-09.html

[Build Bot]

Created attachment 319735 [details]
Archive of layout-test-results from ews105 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 319725 [details]
Patch

Attachment 319725 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/4436324

New failing tests:
imported/w3c/web-platform-tests/html/dom/documents/dom-tree-accessors/document.title-09.html

[Build Bot]

Created attachment 319738 [details]
Archive of layout-test-results from ews113 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews113  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 319725 [details]
Patch

Attachment 319725 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/4436278

New failing tests:
imported/w3c/web-platform-tests/html/dom/documents/dom-tree-accessors/document.title-09.html

[Build Bot]

Created attachment 319739 [details]
Archive of layout-test-results from ews126 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews126  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.5

[Darin Adler]

Created attachment 319820 [details]
Patch

[Darin Adler]

Passing all EWS now. Ready for someone to review.

[Antti Koivisto]

Comment on attachment 319820 [details]
Patch

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

> Source/WebCore/dom/Document.cpp:1574
> +    auto* newTitleElement = m_titleElement.get();
> +    if (is<SVGSVGElement>(documentElement)) {
> +        if (is<SVGTitleElement>(changingTitleElement)) {
> +            if (m_titleElement)
> +                newTitleElement = childrenOfType<SVGTitleElement>(*documentElement).first();
> +            else if (changingTitleElement.parentNode() == documentElement)
> +                newTitleElement = &changingTitleElement;
> +            else
> +                newTitleElement = nullptr;
> +        }
> +        ASSERT(newTitleElement == childrenOfType<SVGTitleElement>(*documentElement).first());
> +    } else {
> +        if (is<HTMLTitleElement>(changingTitleElement)) {
> +            if (m_titleElement)
> +                newTitleElement = descendantsOfType<HTMLTitleElement>(*this).first();
> +            else if (changingTitleElement.isConnected() && !changingTitleElement.isInShadowTree())
> +                newTitleElement = &changingTitleElement;
> +            else
> +                newTitleElement = nullptr;
> +        }
> +        ASSERT(newTitleElement == descendantsOfType<HTMLTitleElement>(*this).first());
>      }

I find this logic somewhat difficult to follow. Maybe having computeNewTitleElementForHTML/SVG helper lambdas would make this cleaner?

> Source/WebCore/rendering/RenderTreeAsText.cpp:964
> +    ASSERT(element); // FIXME: Take a reference instead of a pointer.

Not sure if this sort FIXMEs achieve anything. It is pretty obvious from the code and we have tons of these still.

[Darin Adler]

Comment on attachment 319820 [details]
Patch

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

>> Source/WebCore/dom/Document.cpp:1574
>>      }
> 
> I find this logic somewhat difficult to follow. Maybe having computeNewTitleElementForHTML/SVG helper lambdas would make this cleaner?

I will see what I can do to make it easier to read.

>> Source/WebCore/rendering/RenderTreeAsText.cpp:964
>> +    ASSERT(element); // FIXME: Take a reference instead of a pointer.
> 
> Not sure if this sort FIXMEs achieve anything. It is pretty obvious from the code and we have tons of these still.

OK, dropped it.

[Darin Adler]

Committed r221603: <http://trac.webkit.org/changeset/221603>

[Darin Adler]

Antti, the new version, still a bit complicated, but likely easier to read, is at <https://trac.webkit.org/changeset/221603/webkit#file3>.

[Antti Koivisto]

Interesting solution! Definitely more readable.

[Matt Lewis]

It looks like this revision (r221603) caused the test accessibility/mac/select-element-selection-with-optgroups.html to time out consistently on all macOS platforms.
https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=accessibility%2Fmac%2Fselect-element-selection-with-optgroups.html

giving this diff:

--- /Volumes/Data/slave/sierra-release-tests-wk2/build/layout-test-results/accessibility/mac/select-element-selection-with-optgroups-expected.txt
+++ /Volumes/Data/slave/sierra-release-tests-wk2/build/layout-test-results/accessibility/mac/select-element-selection-with-optgroups-actual.txt
@@ -1,17 +1,5 @@
+#PID UNRESPONSIVE - com.apple.WebKit.WebContent.Development (pid 83065)
+FAIL: Timed out waiting for notifyDone to be called
 
-This tests that setting selection within a list box works correctly if there are optgroups
-
-On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
-
-
-PASS selectElement.selectedChildrenCount is 0
-PASS selectElement.selectedChildrenCount became 1
-PASS selectElement.selectedChildAtIndex(0).isEqual(option1) is true
-PASS selectElement.selectedChildrenCount became 1
-PASS selectElement.selectedChildAtIndex(0).isEqual(option2) is true
-PASS selectElement.selectedChildrenCount became 1
-PASS selectElement.selectedChildAtIndex(0).isEqual(option3) is true
-PASS successfullyParsed is true
-
-TEST COMPLETE
-
+#EOF
+#EOF

[Matt Lewis]

I was able to reproduce this timeout by using run-webkit-tests --no-retry-failures accessibility/mac/select-element-selection-with-optgroups.html

Interestingly when run without the --no-retry-failures argument it passed the first couple of times but once it timed out after passing the argument it consistently timed out with or without it.

[Matt Lewis]

Reverted r221603 for reason:

This caused accessibility/mac/select-element-selection-with-optgroups.html to consistently timeout on macOS

Committed r221626: <http://trac.webkit.org/changeset/221626>

[Darin Adler]

Will re-land after fixing the test.

[Darin Adler]

I figure out what happened. I had the null check logic backwards in AccessibilityUIElement::setSelectedChild and somehow missed the test failure locally. Fixed now!

[Darin Adler]

Committed r221661: <http://trac.webkit.org/changeset/221661>

[Matt Lewis]

Looks like the new patch broke the windows builds:
https://build.webkit.org/builders/Apple%20Win%20Debug%20(Build)/builds/3816
https://build.webkit.org/builders/Apple%20Win%20Debug%20(Build)/builds/3816/steps/compile-webkit/logs/stdio

This probably wasn't seen because the build was broken from another revision when the original patch from this bug was landed. The bad patch was rolled out and now we can see the build break from this revision.

[Matt Lewis]

The first error given:

c:\cygwin\home\buildbot\slave\win-debug\build\source\webcore\dom\Document.cpp(1570): error C2446: ':': no conversion from 'WebCore::Document::updateTitleElement::<lambda_16b62001404c23de7212dde0d19dd8c0>' to 'WebCore::Document::updateTitleElement::<lambda_8c64903d313d33c5fcf4c4945dc4e7bd>' [C:\cygwin\home\buildbot\slave\win-debug\build\WebKitBuild\Debug\Source\WebCore\WebCore.vcxproj]
  c:\cygwin\home\buildbot\slave\win-debug\build\source\webcore\dom\Document.cpp(1570): note: No user-defined-conversion operator available that can perform this conversion, or the operator cannot be called

[Ryan Haddad]

(In reply to Matt Lewis from comment #23)
> Looks like the new patch broke the windows builds:
> https://build.webkit.org/builders/Apple%20Win%20Debug%20(Build)/builds/3816
> https://build.webkit.org/builders/Apple%20Win%20Debug%20(Build)/builds/3816/
> steps/compile-webkit/logs/stdio
> 
> This probably wasn't seen because the build was broken from another revision
> when the original patch from this bug was landed. The bad patch was rolled
> out and now we can see the build break from this revision.

It looks like Per is taking care of this in https://bugs.webkit.org/show_bug.cgi?id=176381

[Radar WebKit Bug Importer]

<rdar://problem/34693717>