Bug 167224

Summary: Remove PassRefPtr from "page" directory of WebCore, also deploy references
Product: WebKit Reporter: Darin Adler <darin>
Component: WebCore Misc.Assignee: Darin Adler <darin>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, buildbot, cdumez, rniwa, sam
Priority: P2    
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
Bug Depends on: 167308    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Patch
none
Archive of layout-test-results from ews103 for mac-elcapitan
none
Archive of layout-test-results from ews107 for mac-elcapitan-wk2
none
Archive of layout-test-results from ews115 for mac-elcapitan
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Archive of layout-test-results from ews100 for mac-elcapitan
none
Patch
none
Archive of layout-test-results from ews102 for mac-elcapitan
none
Patch cdumez: review+

Description Darin Adler 2017-01-19 19:02:20 PST
Remove PassRefPtr from "page" directory of WebCore, also deploy references
Comment 1 Darin Adler 2017-01-19 19:03:55 PST
Created attachment 299300 [details]
Patch
Comment 2 Darin Adler 2017-01-19 19:47:56 PST
Created attachment 299305 [details]
Patch
Comment 3 Build Bot 2017-01-19 20:31:38 PST
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.
Comment 4 Build Bot 2017-01-19 20:31:41 PST
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
Comment 5 Build Bot 2017-01-19 20:34:26 PST
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.
Comment 6 Build Bot 2017-01-19 20:34:31 PST
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
Comment 7 Build Bot 2017-01-19 20:35:01 PST
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.
Comment 8 Build Bot 2017-01-19 20:35:05 PST
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
Comment 9 Darin Adler 2017-01-19 23:30:38 PST
Created attachment 299326 [details]
Patch
Comment 10 Darin Adler 2017-01-19 23:42:08 PST
Created attachment 299328 [details]
Patch
Comment 11 Darin Adler 2017-01-20 19:48:58 PST
Created attachment 299420 [details]
Patch
Comment 12 Darin Adler 2017-01-21 10:59:24 PST
Created attachment 299435 [details]
Patch
Comment 13 Darin Adler 2017-01-21 13:44:15 PST
Created attachment 299442 [details]
Patch
Comment 14 Darin Adler 2017-01-21 13:54:57 PST
Created attachment 299444 [details]
Patch
Comment 15 Darin Adler 2017-01-21 18:28:02 PST
Created attachment 299451 [details]
Patch
Comment 16 Darin Adler 2017-01-21 22:04:58 PST
Created attachment 299457 [details]
Patch
Comment 17 Darin Adler 2017-01-22 02:02:29 PST
Created attachment 299465 [details]
Patch
Comment 18 Build Bot 2017-01-22 03:11:23 PST
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
Comment 19 Build Bot 2017-01-22 03:11:27 PST
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
Comment 20 Darin Adler 2017-01-22 12:55:18 PST
Created attachment 299479 [details]
Patch
Comment 21 Build Bot 2017-01-22 14:02:54 PST
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
Comment 22 Build Bot 2017-01-22 14:02:57 PST
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
Comment 23 Darin Adler 2017-01-22 16:54:59 PST
Created attachment 299489 [details]
Patch
Comment 24 Darin Adler 2017-01-22 19:42:49 PST
Finally, passing all the EWS tests! Now I need a reviewer.
Comment 25 Chris Dumez 2017-01-22 20:38:47 PST
(In reply to comment #24)
> Finally, passing all the EWS tests! Now I need a reviewer.

Looking right now.
Comment 26 Chris Dumez 2017-01-22 21:17:04 PST
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::
Comment 27 Darin Adler 2017-01-22 22:04:38 PST
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.
Comment 28 Darin Adler 2017-01-22 22:25:41 PST
Committed r211033: <http://trac.webkit.org/changeset/211033>
Comment 29 Chris Dumez 2017-01-23 09:42:15 PST
iOS build fix: <http://trac.webkit.org/changeset/211039>