Bug 166569

Summary: Remove PassRefPtr use from the "dom" directory, related cleanup
Product: WebKit Reporter: Darin Adler <darin>
Component: WebCore Misc.Assignee: Darin Adler <darin>
Status: RESOLVED FIXED    
Severity: Normal CC: buildbot, commit-queue, rniwa
Priority: P2    
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
See Also: https://bugs.webkit.org/show_bug.cgi?id=196427
Attachments:
Description Flags
Patch
none
Patch
none
Archive of layout-test-results from ews101 for mac-elcapitan
none
Archive of layout-test-results from ews106 for mac-elcapitan-wk2
none
Archive of layout-test-results from ews117 for mac-elcapitan
none
Patch
none
Patch
none
Archive of layout-test-results from ews102 for mac-elcapitan
none
Archive of layout-test-results from ews106 for mac-elcapitan-wk2
none
Archive of layout-test-results from ews124 for ios-simulator-wk2
none
Archive of layout-test-results from ews112 for mac-elcapitan
none
Archive of layout-test-results from ews117 for mac-elcapitan
none
Patch achristensen: review+

Description Darin Adler 2016-12-28 20:53:02 PST
Remove PassRefPtr use from the "dom" directory, related cleanup
Comment 1 Darin Adler 2016-12-28 22:36:58 PST
Created attachment 297822 [details]
Patch
Comment 2 Darin Adler 2016-12-29 00:04:36 PST
Created attachment 297824 [details]
Patch
Comment 3 WebKit Commit Bot 2016-12-29 00:07:05 PST
Attachment 297824 [details] did not pass style-queue:


ERROR: Source/WebKit/mac/WebCoreSupport/WebFrameLoaderClient.mm:1034:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/dom/MouseEvent.cpp:84:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
ERROR: Source/WebCore/dom/MutationEvent.h:34:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/dom/MutationEvent.h:35:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/dom/MutationEvent.h:36:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
Total errors found: 5 in 70 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 4 Build Bot 2016-12-29 01:16:29 PST
Comment on attachment 297824 [details]
Patch

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

New failing tests:
http/tests/globalhistory/history-delegate-basic-title.html
fast/dom/title-text-property-2.html
Comment 5 Build Bot 2016-12-29 01:16:32 PST
Created attachment 297827 [details]
Archive of layout-test-results from ews101 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-elcapitan  Platform: Mac OS X 10.11.6
Comment 6 Build Bot 2016-12-29 01:22:17 PST
Comment on attachment 297824 [details]
Patch

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

New failing tests:
http/tests/globalhistory/history-delegate-basic-title.html
fast/dom/title-text-property-2.html
Comment 7 Build Bot 2016-12-29 01:22:19 PST
Created attachment 297828 [details]
Archive of layout-test-results from ews106 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6
Comment 8 Build Bot 2016-12-29 01:25:22 PST
Comment on attachment 297824 [details]
Patch

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

New failing tests:
http/tests/globalhistory/history-delegate-basic-title.html
fast/dom/title-text-property-2.html
Comment 9 Build Bot 2016-12-29 01:25:25 PST
Created attachment 297829 [details]
Archive of layout-test-results from ews117 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-elcapitan  Platform: Mac OS X 10.11.6
Comment 10 Darin Adler 2016-12-29 01:28:06 PST
Created attachment 297830 [details]
Patch
Comment 11 WebKit Commit Bot 2016-12-29 01:29:49 PST
Attachment 297830 [details] did not pass style-queue:


ERROR: Source/WebKit/mac/WebCoreSupport/WebFrameLoaderClient.mm:1034:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/dom/MouseEvent.cpp:84:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
ERROR: Source/WebCore/dom/MutationEvent.h:34:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/dom/MutationEvent.h:35:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/dom/MutationEvent.h:36:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
Total errors found: 5 in 72 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 12 Darin Adler 2016-12-30 00:12:04 PST
Created attachment 297849 [details]
Patch
Comment 13 WebKit Commit Bot 2016-12-30 00:13:38 PST
Attachment 297849 [details] did not pass style-queue:


ERROR: Source/WebCore/dom/MouseEvent.cpp:84:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
ERROR: Source/WebKit/mac/WebCoreSupport/WebFrameLoaderClient.mm:1034:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/dom/MutationEvent.h:34:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/dom/MutationEvent.h:35:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/dom/MutationEvent.h:36:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/platform/text/StringWithDirection.h:49:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/WebCore/platform/text/StringWithDirection.h:50:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
Total errors found: 7 in 81 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 14 Build Bot 2016-12-30 01:24:40 PST
Comment on attachment 297849 [details]
Patch

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

New failing tests:
fast/text/mark-matches-rendering.html
fast/text/mark-matches-broken-line-rendering.html
editing/text-iterator/count-mark-simple-lines.html
fast/text/mark-matches-overflow-clip.html
editing/text-iterator/count-mark-lineboxes.html
Comment 15 Build Bot 2016-12-30 01:24:43 PST
Created attachment 297850 [details]
Archive of layout-test-results from ews102 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-elcapitan  Platform: Mac OS X 10.11.6
Comment 16 Build Bot 2016-12-30 01:28:20 PST
Comment on attachment 297849 [details]
Patch

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

New failing tests:
fast/text/mark-matches-rendering.html
fast/text/mark-matches-broken-line-rendering.html
editing/text-iterator/count-mark-simple-lines.html
fast/text/mark-matches-overflow-clip.html
editing/text-iterator/count-mark-lineboxes.html
Comment 17 Build Bot 2016-12-30 01:28:23 PST
Created attachment 297851 [details]
Archive of layout-test-results from ews106 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6
Comment 18 Build Bot 2016-12-30 01:38:56 PST
Comment on attachment 297849 [details]
Patch

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

New failing tests:
editing/text-iterator/count-mark-simple-lines.html
fast/text/mark-matches-overflow-clip.html
editing/text-iterator/count-mark-lineboxes.html
Comment 19 Build Bot 2016-12-30 01:38:59 PST
Created attachment 297852 [details]
Archive of layout-test-results from ews124 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews124  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.6
Comment 20 Build Bot 2016-12-30 02:20:13 PST
Comment on attachment 297849 [details]
Patch

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

New failing tests:
editing/mac/spelling/click-autocorrected-word.html
editing/mac/spelling/delete-autocorrected-word-1.html
editing/mac/spelling/autocorrection-delete.html
imported/blink/editing/apply-inline-style-to-element-with-no-renderer-crash.html
fast/text/mark-matches-rendering.html
editing/text-iterator/count-mark-lineboxes.html
editing/mac/spelling/move-cursor-to-autocorrected-word.html
editing/mac/input/crash-for-empty-text-alternative.html
editing/mac/spelling/autocorrection-simple.html
editing/mac/spelling/delete-autocorrected-word-2.html
editing/mac/input/edit-dictated-text-with-alternative.html
editing/mac/spelling/autocorrection-with-multi-line-text.html
editing/mac/spelling/autocorrection-removing-underline.html
editing/mac/spelling/forward-delete-into-autocorrected-word.html
editing/mac/spelling/move-cursor-to-beginning-of-autocorrected-word.html
editing/mac/spelling/autocorrection-removing-underline-after-paste.html
editing/mac/spelling/autocorrection-blockquote-crash.html
fast/text/mark-matches-broken-line-rendering.html
editing/mac/spelling/removing-underline-after-accepting-autocorrection-using-punctuation.html
editing/text-iterator/count-mark-simple-lines.html
editing/mac/input/insert-dictated-text.html
fast/text/mark-matches-overflow-clip.html
editing/mac/spelling/delete-into-autocorrected-word.html
editing/mac/spelling/accept-candidate-allows-autocorrect-on-next-word.html
editing/mac/spelling/autocorrection-contraction.html
Comment 21 Build Bot 2016-12-30 02:20:16 PST
Created attachment 297853 [details]
Archive of layout-test-results from ews112 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112  Port: mac-elcapitan  Platform: Mac OS X 10.11.6
Comment 22 Build Bot 2016-12-30 04:19:07 PST
Comment on attachment 297849 [details]
Patch

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

New failing tests:
editing/mac/spelling/click-autocorrected-word.html
editing/mac/spelling/delete-autocorrected-word-1.html
editing/mac/spelling/autocorrection-delete.html
imported/blink/editing/apply-inline-style-to-element-with-no-renderer-crash.html
fast/text/mark-matches-rendering.html
editing/text-iterator/count-mark-lineboxes.html
editing/mac/spelling/move-cursor-to-autocorrected-word.html
editing/mac/input/crash-for-empty-text-alternative.html
editing/mac/spelling/autocorrection-simple.html
editing/mac/spelling/delete-autocorrected-word-2.html
editing/mac/input/edit-dictated-text-with-alternative.html
editing/mac/spelling/autocorrection-with-multi-line-text.html
editing/mac/spelling/autocorrection-removing-underline.html
editing/mac/spelling/forward-delete-into-autocorrected-word.html
editing/mac/spelling/move-cursor-to-beginning-of-autocorrected-word.html
editing/mac/spelling/autocorrection-removing-underline-after-paste.html
editing/mac/spelling/autocorrection-blockquote-crash.html
fast/text/mark-matches-broken-line-rendering.html
editing/mac/spelling/removing-underline-after-accepting-autocorrection-using-punctuation.html
editing/text-iterator/count-mark-simple-lines.html
editing/mac/input/insert-dictated-text.html
fast/text/mark-matches-overflow-clip.html
editing/mac/spelling/delete-into-autocorrected-word.html
editing/mac/spelling/accept-candidate-allows-autocorrect-on-next-word.html
editing/mac/spelling/autocorrection-contraction.html
Comment 23 Build Bot 2016-12-30 04:19:13 PST
Created attachment 297855 [details]
Archive of layout-test-results from ews117 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-elcapitan  Platform: Mac OS X 10.11.6
Comment 24 Darin Adler 2016-12-30 19:02:43 PST
Created attachment 297866 [details]
Patch
Comment 25 WebKit Commit Bot 2016-12-30 19:04:31 PST
Attachment 297866 [details] did not pass style-queue:


ERROR: Source/WebCore/dom/MouseEvent.cpp:84:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
ERROR: Source/WebKit/mac/WebCoreSupport/WebFrameLoaderClient.mm:1034:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/dom/MutationEvent.h:34:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/dom/MutationEvent.h:35:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/dom/MutationEvent.h:36:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/platform/text/StringWithDirection.h:49:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/WebCore/platform/text/StringWithDirection.h:50:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
Total errors found: 7 in 84 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 26 Alex Christensen 2016-12-30 21:54:06 PST
Comment on attachment 297866 [details]
Patch

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

> Source/WebCore/ChangeLog:458
> +        * rendering/HitTestResult.cpp:

This ChangeLog entry seems out of place.

> Source/WebCore/dom/DOMNamedFlowCollection.cpp:-67
> -    // FIXME: Should be implemented.

You implemented this.  Can it be tested?

> Source/WebCore/dom/DOMNamedFlowCollection.idl:39
> -    getter WebKitNamedFlow item(unsigned long index);
> -    getter WebKitNamedFlow namedItem(DOMString name);
> +    getter WebKitNamedFlow? item(unsigned long index);
> +    getter WebKitNamedFlow? namedItem(DOMString name);

Can this change be tested?

> Source/WebCore/dom/DocumentMarker.h:99
> +            : MarkerTypes(0

This 0 doesn't seem useful.

> Source/WebCore/editing/AlternativeTextController.cpp:731
>      Vector<RenderedDocumentMarker*> dictationAlternativesMarkers = markers.markersInRange(selection.get(), DocumentMarker::DictationAlternatives);

auto
Also, could we make markersInRange and markersFor return Vector<std::reference_wrapper<RenderedDocumentMarker>> to make it clear that they can't  be null?  If we only iterate them and count them, we might be able to get away with using clever iterator types and never allocating a Vector.
Comment 27 Darin Adler 2016-12-31 01:10:38 PST
Comment on attachment 297866 [details]
Patch

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

>> Source/WebCore/ChangeLog:458
>> +        * rendering/HitTestResult.cpp:
> 
> This ChangeLog entry seems out of place.

Oops! Will fix this.

>> Source/WebCore/dom/DOMNamedFlowCollection.cpp:-67
>> -    // FIXME: Should be implemented.
> 
> You implemented this.  Can it be tested?

Good point. I should definitely add a test even though I don’t already know how. I will figure it out.

>> Source/WebCore/dom/DOMNamedFlowCollection.idl:39
>> +    getter WebKitNamedFlow? namedItem(DOMString name);
> 
> Can this change be tested?

No, there is no change in behavior here. When we specify a return value that can’t be null in an IDL file, our generated code can handle null just fine, and vice versa, so we can get this wrong in IDL files with no repercussions.

>> Source/WebCore/dom/DocumentMarker.h:99
>> +            : MarkerTypes(0
> 
> This 0 doesn't seem useful.

It is here so that the subsequent lines can all have a "|" at the start of them, rather than having one special one without a "|".

>> Source/WebCore/editing/AlternativeTextController.cpp:731
>>      Vector<RenderedDocumentMarker*> dictationAlternativesMarkers = markers.markersInRange(selection.get(), DocumentMarker::DictationAlternatives);
> 
> auto
> Also, could we make markersInRange and markersFor return Vector<std::reference_wrapper<RenderedDocumentMarker>> to make it clear that they can't  be null?  If we only iterate them and count them, we might be able to get away with using clever iterator types and never allocating a Vector.

Sure, we could do those things. I prefer not to do them in this patch, though.
Comment 28 Darin Adler 2016-12-31 01:52:24 PST
Committed r210216: <http://trac.webkit.org/changeset/210216>