Bug 179218 - Replace some auto* with RefPtr within WebCore/html
Summary: Replace some auto* with RefPtr within WebCore/html
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Jiewen Tan
URL:
Keywords: InRadar
Depends on:
Blocks: 179844
  Show dependency treegraph
 
Reported: 2017-11-02 17:26 PDT by Jiewen Tan
Modified: 2017-11-17 17:04 PST (History)
7 users (show)

See Also:


Attachments
Patch (101.46 KB, patch)
2017-11-02 17:40 PDT, Jiewen Tan
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews102 for mac-elcapitan (526.41 KB, application/zip)
2017-11-02 18:25 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews105 for mac-elcapitan-wk2 (675.94 KB, application/zip)
2017-11-02 18:30 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews117 for mac-elcapitan (761.11 KB, application/zip)
2017-11-02 18:32 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews122 for ios-simulator-wk2 (1.01 MB, application/zip)
2017-11-02 18:37 PDT, Build Bot
no flags Details
Patch (102.51 KB, patch)
2017-11-02 19:29 PDT, Jiewen Tan
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews101 for mac-elcapitan (770.64 KB, application/zip)
2017-11-02 20:12 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews107 for mac-elcapitan-wk2 (845.85 KB, application/zip)
2017-11-02 20:17 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews116 for mac-elcapitan (708.96 KB, application/zip)
2017-11-02 20:25 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews121 for ios-simulator-wk2 (903.84 KB, application/zip)
2017-11-02 20:35 PDT, Build Bot
no flags Details
Patch (102.15 KB, patch)
2017-11-02 21:02 PDT, Jiewen Tan
rniwa: review+
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews105 for mac-elcapitan-wk2 (1.79 MB, application/zip)
2017-11-02 21:47 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews102 for mac-elcapitan (1.01 MB, application/zip)
2017-11-02 21:50 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews114 for mac-elcapitan (818.58 KB, application/zip)
2017-11-02 21:55 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews124 for ios-simulator-wk2 (1.30 MB, application/zip)
2017-11-02 22:13 PDT, Build Bot
no flags Details
Patch for landing (101.40 KB, patch)
2017-11-03 01:11 PDT, Jiewen Tan
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jiewen Tan 2017-11-02 17:26:48 PDT
Convert auto* to RefPtr within WebCore/html if appropriate.
Comment 1 Jiewen Tan 2017-11-02 17:27:49 PDT
<rdar://problem/35102567>
Comment 2 Jiewen Tan 2017-11-02 17:40:44 PDT
Created attachment 325804 [details]
Patch
Comment 3 Build Bot 2017-11-02 18:25:29 PDT Comment hidden (obsolete)
Comment 4 Build Bot 2017-11-02 18:25:30 PDT Comment hidden (obsolete)
Comment 5 Build Bot 2017-11-02 18:30:27 PDT Comment hidden (obsolete)
Comment 6 Build Bot 2017-11-02 18:30:28 PDT Comment hidden (obsolete)
Comment 7 Build Bot 2017-11-02 18:32:06 PDT Comment hidden (obsolete)
Comment 8 Build Bot 2017-11-02 18:32:07 PDT Comment hidden (obsolete)
Comment 9 Build Bot 2017-11-02 18:37:47 PDT Comment hidden (obsolete)
Comment 10 Build Bot 2017-11-02 18:37:48 PDT Comment hidden (obsolete)
Comment 11 Jiewen Tan 2017-11-02 19:29:31 PDT
Created attachment 325822 [details]
Patch
Comment 12 Build Bot 2017-11-02 20:12:06 PDT Comment hidden (obsolete)
Comment 13 Build Bot 2017-11-02 20:12:07 PDT Comment hidden (obsolete)
Comment 14 Build Bot 2017-11-02 20:17:28 PDT Comment hidden (obsolete)
Comment 15 Build Bot 2017-11-02 20:17:29 PDT Comment hidden (obsolete)
Comment 16 Build Bot 2017-11-02 20:25:54 PDT Comment hidden (obsolete)
Comment 17 Build Bot 2017-11-02 20:25:56 PDT Comment hidden (obsolete)
Comment 18 Build Bot 2017-11-02 20:35:46 PDT Comment hidden (obsolete)
Comment 19 Build Bot 2017-11-02 20:35:47 PDT Comment hidden (obsolete)
Comment 20 Jiewen Tan 2017-11-02 21:02:39 PDT
Created attachment 325839 [details]
Patch
Comment 21 Ryosuke Niwa 2017-11-02 21:14:34 PDT
Comment on attachment 325839 [details]
Patch

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

> Source/WebCore/accessibility/AXObjectCache.cpp:1526
> +    RefPtr<HTMLElement> correspondingControl = downcast<HTMLLabelElement>(*element).control();

We can't use makeRefPtr & auto here?

> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:2527
> -    HTMLElement* correspondingControl = labelElement->control();
> +    RefPtr<HTMLElement> correspondingControl = labelElement->control();

Ditto.
Comment 22 Ryosuke Niwa 2017-11-02 21:24:08 PDT
Comment on attachment 325839 [details]
Patch

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

> Source/WebCore/accessibility/AccessibilityTable.cpp:396
> +        if (RefPtr<HTMLTableCaptionElement> caption = tableElement->caption()) {

Use auto?

> Source/WebCore/accessibility/AccessibilityTable.cpp:690
> -        if (HTMLTableCaptionElement* caption = downcast<HTMLTableElement>(*tableElement).caption())
> +        if (RefPtr<HTMLTableCaptionElement> caption = downcast<HTMLTableElement>(*tableElement).caption())

Ditto.

> Source/WebCore/html/HTMLSummaryElement.cpp:76
> +    auto parent = makeRefPtr(parentElement());

It's a bit silly to use a RefPtr here since we're just downcast'ing to another pointer type.
I think it's okay to keep parent & host raw pointers as long as the return type is a RefPtr.

> Source/WebCore/html/HTMLTableRowElement.cpp:59
> -    auto* parent = row.parentNode();
> +    auto parent = makeRefPtr(row.parentNode());

Again, not certain if it's worth converting these local variables to RefPtr since they're just casted to another pointer type.
Comment 23 Jiewen Tan 2017-11-02 21:43:52 PDT
Comment on attachment 325839 [details]
Patch

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

Thanks Ryosuke for r+ my patch. I will fix all crashes before landing the patch.

>> Source/WebCore/accessibility/AXObjectCache.cpp:1526
>> +    RefPtr<HTMLElement> correspondingControl = downcast<HTMLLabelElement>(*element).control();
> 
> We can't use makeRefPtr & auto here?

Fixed.

>> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:2527
>> +    RefPtr<HTMLElement> correspondingControl = labelElement->control();
> 
> Ditto.

Fixed.

>> Source/WebCore/accessibility/AccessibilityTable.cpp:396
>> +        if (RefPtr<HTMLTableCaptionElement> caption = tableElement->caption()) {
> 
> Use auto?

Fixed.

>> Source/WebCore/accessibility/AccessibilityTable.cpp:690
>> +        if (RefPtr<HTMLTableCaptionElement> caption = downcast<HTMLTableElement>(*tableElement).caption())
> 
> Ditto.

Fixed.

>> Source/WebCore/html/HTMLSummaryElement.cpp:76
>> +    auto parent = makeRefPtr(parentElement());
> 
> It's a bit silly to use a RefPtr here since we're just downcast'ing to another pointer type.
> I think it's okay to keep parent & host raw pointers as long as the return type is a RefPtr.

Fixed.

>> Source/WebCore/html/HTMLTableRowElement.cpp:59
>> +    auto parent = makeRefPtr(row.parentNode());
> 
> Again, not certain if it's worth converting these local variables to RefPtr since they're just casted to another pointer type.

Fixed.
Comment 24 Build Bot 2017-11-02 21:47:39 PDT Comment hidden (obsolete)
Comment 25 Build Bot 2017-11-02 21:47:40 PDT Comment hidden (obsolete)
Comment 26 Build Bot 2017-11-02 21:50:18 PDT Comment hidden (obsolete)
Comment 27 Build Bot 2017-11-02 21:50:20 PDT Comment hidden (obsolete)
Comment 28 Build Bot 2017-11-02 21:55:15 PDT Comment hidden (obsolete)
Comment 29 Build Bot 2017-11-02 21:55:16 PDT Comment hidden (obsolete)
Comment 30 Build Bot 2017-11-02 22:13:31 PDT Comment hidden (obsolete)
Comment 31 Build Bot 2017-11-02 22:13:32 PDT Comment hidden (obsolete)
Comment 32 Jiewen Tan 2017-11-03 01:11:59 PDT
Created attachment 325874 [details]
Patch for landing
Comment 33 WebKit Commit Bot 2017-11-03 02:32:09 PDT
Comment on attachment 325874 [details]
Patch for landing

Clearing flags on attachment: 325874

Committed r224390: <https://trac.webkit.org/changeset/224390>
Comment 34 zalan 2017-11-17 17:04:03 PST
(In reply to WebKit Commit Bot from comment #33)
> Comment on attachment 325874 [details]
> Patch for landing
> 
> Clearing flags on attachment: 325874
> 
> Committed r224390: <https://trac.webkit.org/changeset/224390>
regression -> bug 179854