Bug 167224 – Remove PassRefPtr from "page" directory of WebCore, also deploy references

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

[Darin Adler]

Remove PassRefPtr from "page" directory of WebCore, also deploy references

[Darin Adler]

Created attachment 299300 [details]
Patch

[Darin Adler]

Created attachment 299305 [details]
Patch

[Build Bot]

Comment on attachment 299305 [details]
Patch

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

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 299306 [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 299305 [details]
Patch

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

Number of test failures exceeded the failure limit.

[Build Bot]

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

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

[Build Bot]

Comment on attachment 299305 [details]
Patch

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

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 299310 [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 299326 [details]
Patch

[Darin Adler]

Created attachment 299328 [details]
Patch

[Darin Adler]

Created attachment 299420 [details]
Patch

[Darin Adler]

Created attachment 299435 [details]
Patch

[Darin Adler]

Created attachment 299442 [details]
Patch

[Darin Adler]

Created attachment 299444 [details]
Patch

[Darin Adler]

Created attachment 299451 [details]
Patch

[Darin Adler]

Created attachment 299457 [details]
Patch

[Darin Adler]

Created attachment 299465 [details]
Patch

[Build Bot]

Comment on attachment 299465 [details]
Patch

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

New failing tests:
media/audio-concurrent-supported.html
imported/w3c/web-platform-tests/html/browsers/history/the-location-interface/location-protocol-setter-non-broken.html

[Build Bot]

Created attachment 299466 [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

[Darin Adler]

Created attachment 299479 [details]
Patch

[Build Bot]

Comment on attachment 299479 [details]
Patch

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

New failing tests:
imported/w3c/web-platform-tests/html/browsers/history/the-location-interface/location-protocol-setter-non-broken.html

[Build Bot]

Created attachment 299483 [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

[Darin Adler]

Created attachment 299489 [details]
Patch

[Darin Adler]

Finally, passing all the EWS tests! Now I need a reviewer.

[Chris Dumez]

(In reply to comment #24)
> Finally, passing all the EWS tests! Now I need a reviewer.

Looking right now.

[Chris Dumez]

Comment on attachment 299489 [details]
Patch

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

r=me

> Source/WebCore/loader/FrameLoader.cpp:1856
> +        if (auto* page = m_frame.page())

Why did we drop the m_frame.document()->doctype() null check? It seems we now call didReceiveDocType() even if we have no doctype.

> Source/WebCore/page/DebugPageOverlays.cpp:146
> +RefPtr<RegionOverlay> RegionOverlay::create(MainFrame& frame, DebugPageOverlays::RegionType regionType)

Can this return a Ref? The return nullptr below seems useless since the switch handles all enum values.

> Source/WebCore/page/DragController.cpp:198
>          RefPtr<DataTransfer> dataTransfer = DataTransfer::createForDragAndDrop(policy, dragData);

Should be a Ref<> or auto. createForDragAndDrop() returns a Ref<>

> Source/WebCore/page/DragController.cpp:228
>              RefPtr<DataTransfer> dataTransfer = DataTransfer::createForDragAndDrop(DataTransferAccessPolicy::Readable, dragData);

Should be a Ref<> or auto. createForDragAndDrop() returns a Ref<>

> Source/WebCore/page/PerformanceUserTiming.cpp:92
> +    RefPtr<PerformanceEntry> entry = WTFMove(performanceEntry);

Why use a RefPtr here? Can't we use a Ref<> here and releaseNonNull() below?

> Source/WebKit/win/WebView.cpp:5856
> +        m_page->focusController().setFocusedElement(nullptr, m_page->focusController().focusedOrMainFrame());

Why this change?

> Source/WebKit2/WebProcess/WebCoreSupport/WebChromeClient.cpp:182
> +void WebChromeClient::elementDidFocus(WebCore::Element& element)

No need for WebCore::

> Source/WebKit2/WebProcess/WebCoreSupport/WebChromeClient.cpp:187
> +void WebChromeClient::elementDidBlur(WebCore::Element& element)

No need for WebCore::

[Darin Adler]

Comment on attachment 299489 [details]
Patch

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

Thanks for the review.

>> Source/WebCore/loader/FrameLoader.cpp:1856
>> +        if (auto* page = m_frame.page())
> 
> Why did we drop the m_frame.document()->doctype() null check? It seems we now call didReceiveDocType() even if we have no doctype.

The other call sites for didReceiveDocType, in Document::childrenChanged and Document::updateViewportArguments, are both unconditional. This was the only one checking for a doctype. And if you look at the implementation, didReceiveDocType will call didReceiveMobileDocType with a boolean that correctly is based on whether a doctype is present as well as its contents.

I made this call site for loading from a cached page consistent with the other call sites, and I think it is likely a progression even though I did not create a new test case.

>> Source/WebCore/page/DebugPageOverlays.cpp:146
>> +RefPtr<RegionOverlay> RegionOverlay::create(MainFrame& frame, DebugPageOverlays::RegionType regionType)
> 
> Can this return a Ref? The return nullptr below seems useless since the switch handles all enum values.

Sure. It’s local to this file. It’s sometimes a pain to make something with a switch return a Ref because we have to write the unreachable code to compile correctly with our various compilers, but I will do that.

>> Source/WebCore/page/DragController.cpp:198
>>          RefPtr<DataTransfer> dataTransfer = DataTransfer::createForDragAndDrop(policy, dragData);
> 
> Should be a Ref<> or auto. createForDragAndDrop() returns a Ref<>

OK, done.

>> Source/WebCore/page/DragController.cpp:228
>>              RefPtr<DataTransfer> dataTransfer = DataTransfer::createForDragAndDrop(DataTransferAccessPolicy::Readable, dragData);
> 
> Should be a Ref<> or auto. createForDragAndDrop() returns a Ref<>

OK, done.

>> Source/WebCore/page/PerformanceUserTiming.cpp:92
>> +    RefPtr<PerformanceEntry> entry = WTFMove(performanceEntry);
> 
> Why use a RefPtr here? Can't we use a Ref<> here and releaseNonNull() below?

I am going to just leave this one alone for now. Changing this to a Ref would make the Vector definition below tricky. And, also, this code is disabled on Mac. I’d be fine if someone wants to come refine this later, but I prefer not to do that right now.

>> Source/WebKit/win/WebView.cpp:5856
>> +        m_page->focusController().setFocusedElement(nullptr, m_page->focusController().focusedOrMainFrame());
> 
> Why this change?

I changed setFocusedElement to take a reference for the frame, not a pointer. I could have kept it taking a pointer just for this one Windows-only code path, but I prefer not to. No code for any other platform needs to pass null for the frame; I do not think Windows needs to.

In fact, looking at it further, I think this is unused code on Windows. It’s in IWebViewPrivate.idl so it was exposed, probably for Safari on Windows, but I bet it is unused now and someone working on Windows can just delete this or perhaps empty out the function and make it do nothing.

>> Source/WebKit2/WebProcess/WebCoreSupport/WebChromeClient.cpp:182
>> +void WebChromeClient::elementDidFocus(WebCore::Element& element)
> 
> No need for WebCore::

Took care of this. There were lots of other unneeded WebCore prefixes in this file; removed those too.

>> Source/WebKit2/WebProcess/WebCoreSupport/WebChromeClient.cpp:187
>> +void WebChromeClient::elementDidBlur(WebCore::Element& element)
> 
> No need for WebCore::

Ditto.

[Darin Adler]

Committed r211033: <http://trac.webkit.org/changeset/211033>

[Chris Dumez]

iOS build fix: <http://trac.webkit.org/changeset/211039>