Bug 166635 – Remove PassRefPtr use from the "html" directory, other improvements

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Darin Adler]

Remove PassRefPtr use from the "html" directory, other improvements

[Darin Adler]

Created attachment 297880 [details]
Patch

[Darin Adler]

Created attachment 297882 [details]
Patch

[Build Bot]

Comment on attachment 297882 [details]
Patch

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

New failing tests:
fast/forms/file/input-file-write-files.html

[Build Bot]

Created attachment 297884 [details]
Archive of layout-test-results from ews103 for mac-elcapitan

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

[Build Bot]

Comment on attachment 297882 [details]
Patch

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

New failing tests:
fast/forms/file/input-file-write-files.html

[Build Bot]

Created attachment 297886 [details]
Archive of layout-test-results from ews114 for mac-elcapitan

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

[Darin Adler]

Created attachment 297904 [details]
Patch

[Darin Adler]

Created attachment 297919 [details]
Patch

[Darin Adler]

Created attachment 297921 [details]
Patch

[Darin Adler]

Everything building and passing tests. Now I just need to write change log and decide whether to break this up into multiple patches or get it reviewed as one large one.

[Darin Adler]

Comment on attachment 297921 [details]
Patch

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

> Source/WebCore/html/track/TextTrackCueList.cpp:102
> +    unsigned insertionPosition = std::upper_bound(m_vector.begin(), m_vector.end(), cueRefPtr, compareCues) - m_vector.begin();
> +    ASSERT_SORTED(m_vector.begin(), m_vector.end(), compareCues);
> +    m_vector.insert(insertionPosition, WTFMove(cueRefPtr));
> +    ASSERT_SORTED(m_vector.begin(), m_vector.end(), compareCues);

Note to self: Old code would refuse to add duplicates (and returned false, but all callers were ignoring that return value). Need to either restore that behavior and make sure a regression test covers that, or figure out why it’s not actually a reachable case.

[Alex Christensen]

Comment on attachment 297921 [details]
Patch

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

I'll review a large patch.  A lot of this would be hard to split up into smaller patches.

> Source/WebCore/CMakeLists.txt:-1806
> -    html/canvas/WebGLQuery.cpp

Let's actually remove all these files.

[Darin Adler]

Comment on attachment 297921 [details]
Patch

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

>> Source/WebCore/CMakeLists.txt:-1806
>> -    html/canvas/WebGLQuery.cpp
> 
> Let's actually remove all these files.

I’m not sure what you mean. With the exception of WebGLGetInfo.cpp, these files are all used when ENABLE_WEBGL is true. The reason I deleted them from this list is that they were listed in both the main sources list and in the ENABLE_WEBGL list, and there was no need to list them twice, and no need to compile them when WebGL is not enabled. I discovered this because I had forgotten to put an #if in WebGLAny.cpp and I put that in both lists. It failed to compile on Windows. So I fixed it twice — once by not adding it to this list and a second time by adding an appropriate #if.

[Darin Adler]

Created attachment 297994 [details]
Patch

[Darin Adler]

Created attachment 297995 [details]
Patch

[Darin Adler]

Created attachment 298001 [details]
Patch

[WebKit Commit Bot]

Attachment 298001 [details] did not pass style-queue:


ERROR: Source/WebCore/html/canvas/WebGLRenderingContextBase.h:575:  'getWebGLFloatArrayParameter' is incorrectly named. It should be named 'protector' or 'protectedGC3Denum'.  [readability/naming/protected] [4]
ERROR: Source/WebCore/html/canvas/WebGLRenderingContextBase.h:576:  'getWebGLIntArrayParameter' is incorrectly named. It should be named 'protector' or 'protectedGC3Denum'.  [readability/naming/protected] [4]
Total errors found: 2 in 171 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Darin Adler]

Created attachment 298015 [details]
Patch

[Darin Adler]

(In reply to comment #17)
> ERROR: Source/WebCore/html/canvas/WebGLRenderingContextBase.h:575: 
> 'getWebGLFloatArrayParameter' is incorrectly named. It should be named
> 'protector' or 'protectedGC3Denum'.  [readability/naming/protected] [4]
> ERROR: Source/WebCore/html/canvas/WebGLRenderingContextBase.h:576: 
> 'getWebGLIntArrayParameter' is incorrectly named. It should be named
> 'protector' or 'protectedGC3Denum'.  [readability/naming/protected] [4]

Brady, this failure is annoying.

[WebKit Commit Bot]

Attachment 298015 [details] did not pass style-queue:


ERROR: Source/WebCore/html/canvas/WebGLRenderingContextBase.h:575:  'getWebGLFloatArrayParameter' is incorrectly named. It should be named 'protector' or 'protectedGC3Denum'.  [readability/naming/protected] [4]
ERROR: Source/WebCore/html/canvas/WebGLRenderingContextBase.h:576:  'getWebGLIntArrayParameter' is incorrectly named. It should be named 'protector' or 'protectedGC3Denum'.  [readability/naming/protected] [4]
Total errors found: 2 in 171 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Darin Adler]

Comment on attachment 298015 [details]
Patch

Need to fix the build failure on Windows (done) and the crash seen in the track tests (will probably have to wait until tonight).

[Darin Adler]

Created attachment 298025 [details]
Patch

[WebKit Commit Bot]

Attachment 298025 [details] did not pass style-queue:


ERROR: Source/WebCore/html/canvas/WebGLRenderingContextBase.h:575:  'getWebGLFloatArrayParameter' is incorrectly named. It should be named 'protector' or 'protectedGC3Denum'.  [readability/naming/protected] [4]
ERROR: Source/WebCore/html/canvas/WebGLRenderingContextBase.h:576:  'getWebGLIntArrayParameter' is incorrectly named. It should be named 'protector' or 'protectedGC3Denum'.  [readability/naming/protected] [4]
Total errors found: 2 in 171 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Brady Eidson]

(In reply to comment #19)
> (In reply to comment #17)
> > ERROR: Source/WebCore/html/canvas/WebGLRenderingContextBase.h:575: 
> > 'getWebGLFloatArrayParameter' is incorrectly named. It should be named
> > 'protector' or 'protectedGC3Denum'.  [readability/naming/protected] [4]
> > ERROR: Source/WebCore/html/canvas/WebGLRenderingContextBase.h:576: 
> > 'getWebGLIntArrayParameter' is incorrectly named. It should be named
> > 'protector' or 'protectedGC3Denum'.  [readability/naming/protected] [4]
> 
> Brady, this failure is annoying.

I agree.

And it's also fixable, but I haven't been able to navigate our scripts with my extremely limited python expertise to do so.

In the past I've tried convince others who are way more proficient than myself to take the time to help but have not been successful.

[Alex Christensen]

Comment on attachment 298025 [details]
Patch

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

> Source/WebCore/html/track/TextTrackCueList.cpp:98
> +    unsigned insertionPosition = std::upper_bound(m_vector.begin(), m_vector.end(), cueRefPtr, compareCues) - m_vector.begin();
> +    ASSERT_SORTED(m_vector.begin(), m_vector.end());
> +    m_vector.insert(insertionPosition, WTFMove(cueRefPtr));

If you insert near the beginning every time, this could cause O(n^2) execution time.

[Darin Adler]

Comment on attachment 298025 [details]
Patch

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

>> Source/WebCore/html/track/TextTrackCueList.cpp:98
>> +    m_vector.insert(insertionPosition, WTFMove(cueRefPtr));
> 
> If you insert near the beginning every time, this could cause O(n^2) execution time.

Yes, true, and not new to this patch.

[Darin Adler]

Created attachment 298057 [details]
Patch

[WebKit Commit Bot]

Attachment 298057 [details] did not pass style-queue:


ERROR: Source/WebCore/html/canvas/WebGLRenderingContextBase.h:575:  'getWebGLFloatArrayParameter' is incorrectly named. It should be named 'protector' or 'protectedGC3Denum'.  [readability/naming/protected] [4]
ERROR: Source/WebCore/html/canvas/WebGLRenderingContextBase.h:576:  'getWebGLIntArrayParameter' is incorrectly named. It should be named 'protector' or 'protectedGC3Denum'.  [readability/naming/protected] [4]
Total errors found: 2 in 171 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 298057 [details]
Patch

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

New failing tests:
imported/w3c/web-platform-tests/html/semantics/embedded-content/media-elements/interfaces/TextTrack/addCue.html
media/track/w3c/interfaces/TextTrack/addCue.html

[Build Bot]

Created attachment 298063 [details]
Archive of layout-test-results from ews115 for mac-elcapitan

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

[Darin Adler]

Created attachment 298066 [details]
Patch

[WebKit Commit Bot]

Attachment 298066 [details] did not pass style-queue:


ERROR: Source/WebCore/html/canvas/WebGLRenderingContextBase.h:575:  'getWebGLFloatArrayParameter' is incorrectly named. It should be named 'protector' or 'protectedGC3Denum'.  [readability/naming/protected] [4]
ERROR: Source/WebCore/html/canvas/WebGLRenderingContextBase.h:576:  'getWebGLIntArrayParameter' is incorrectly named. It should be named 'protector' or 'protectedGC3Denum'.  [readability/naming/protected] [4]
Total errors found: 2 in 171 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Alex Christensen]

Comment on attachment 298066 [details]
Patch

r=me!  Woo hoo!

[WebKit Commit Bot]

Comment on attachment 298066 [details]
Patch

Clearing flags on attachment: 298066

Committed r210319: <http://trac.webkit.org/changeset/210319>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Csaba Osztrogonác]

(In reply to comment #34)
> Comment on attachment 298066 [details]
> Patch
> 
> Clearing flags on attachment: 298066
> 
> Committed r210319: <http://trac.webkit.org/changeset/210319>

It broke the WinCairo build :https://build.webkit.org/builders/WinCairo%2064-Bit%20Release/builds/63994

[Darin Adler]

(In reply to comment #36)
> It broke the WinCairo build
> https://build.webkit.org/builders/WinCairo%2064-Bit%20Release/builds/63994

Is there someone who has a WinCairo configuration who can try different fixes for this?

[Alex Christensen]

http://trac.webkit.org/changeset/210465