Bug 191119 - In WebCore::ResourceLoadObserver, use document.sessionID().isEphemeral() when possible and check for page existence when not
Summary: In WebCore::ResourceLoadObserver, use document.sessionID().isEphemeral() when...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: John Wilander
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-10-31 10:34 PDT by John Wilander
Modified: 2018-11-01 17:51 PDT (History)
6 users (show)

See Also:


Attachments
Patch (2.03 KB, patch)
2018-10-31 12:03 PDT, John Wilander
no flags Details | Formatted Diff | Diff
Patch (11.28 KB, patch)
2018-11-01 15:11 PDT, John Wilander
no flags Details | Formatted Diff | Diff
Patch (11.38 KB, patch)
2018-11-01 15:40 PDT, John Wilander
no flags Details | Formatted Diff | Diff
Patch for landing (11.32 KB, patch)
2018-11-01 17:14 PDT, John Wilander
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description John Wilander 2018-10-31 10:34:29 PDT
https://bugs.webkit.org/show_bug.cgi?id=188757 restructured the calls to ResourceLoadObserver::shouldLog(). Now we need to either get the boolean parameter from the document or check the validity of the page.
Comment 1 John Wilander 2018-10-31 10:34:53 PDT
<rdar://problem/44176965>
Comment 2 John Wilander 2018-10-31 12:03:34 PDT
Created attachment 353517 [details]
Patch
Comment 3 Chris Dumez 2018-10-31 12:14:05 PDT
Comment on attachment 353517 [details]
Patch

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

> Source/WebCore/ChangeLog:9
> +        No new tests. The change just makes use of a reference instead of a pointer

We should be able to write an API test for this:
1. Create a Legacy Mac WebView
2. Call _close on it
3. Call keyDown on it
Comment 4 John Wilander 2018-11-01 15:11:42 PDT
Created attachment 353649 [details]
Patch
Comment 5 EWS Watchlist 2018-11-01 15:14:00 PDT
Attachment 353649 [details] did not pass style-queue:


ERROR: Tools/TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj:3581:  developmentRegion is not en.  [xcodeproj/settings] [5]
ERROR: Tools/TestWebKitAPI/Tests/WebKitLegacy/mac/ClosingWebView.mm:86:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Tools/TestWebKitAPI/Tests/WebKitLegacy/mac/ClosingWebView.mm:89:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Tools/TestWebKitAPI/Tests/WebKitLegacy/mac/ClosingWebView.mm:90:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Tools/TestWebKitAPI/Tests/WebKitLegacy/mac/ClosingWebView.mm:91:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Tools/TestWebKitAPI/Tests/WebKitLegacy/mac/ClosingWebView.mm:92:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Tools/TestWebKitAPI/Tests/WebKitLegacy/mac/ClosingWebView.mm:93:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Tools/TestWebKitAPI/Tests/WebKitLegacy/mac/ClosingWebView.mm:94:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 8 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 6 John Wilander 2018-11-01 15:40:29 PDT
Created attachment 353654 [details]
Patch
Comment 7 EWS Watchlist 2018-11-01 15:42:45 PDT
Attachment 353654 [details] did not pass style-queue:


ERROR: Tools/TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj:3581:  developmentRegion is not en.  [xcodeproj/settings] [5]
ERROR: Tools/TestWebKitAPI/Tests/WebKitLegacy/mac/ClosingWebView.mm:89:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Tools/TestWebKitAPI/Tests/WebKitLegacy/mac/ClosingWebView.mm:92:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Tools/TestWebKitAPI/Tests/WebKitLegacy/mac/ClosingWebView.mm:93:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Tools/TestWebKitAPI/Tests/WebKitLegacy/mac/ClosingWebView.mm:94:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Tools/TestWebKitAPI/Tests/WebKitLegacy/mac/ClosingWebView.mm:95:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Tools/TestWebKitAPI/Tests/WebKitLegacy/mac/ClosingWebView.mm:96:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Tools/TestWebKitAPI/Tests/WebKitLegacy/mac/ClosingWebView.mm:97:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 8 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 8 John Wilander 2018-11-01 15:49:20 PDT
(In reply to Build Bot from comment #7)
> Attachment 353654 [details] did not pass style-queue:
> 
> 
> ERROR: Tools/TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj:3581: 
> developmentRegion is not en.  [xcodeproj/settings] [5]

It says "English" instead of "en." My patch does not change this value.

> ERROR: Tools/TestWebKitAPI/Tests/WebKitLegacy/mac/ClosingWebView.mm:89: 
> Weird number of spaces at line-start.  Are you using a 4-space indent? 
> [whitespace/indent] [3]
> ERROR: Tools/TestWebKitAPI/Tests/WebKitLegacy/mac/ClosingWebView.mm:92: 
> Weird number of spaces at line-start.  Are you using a 4-space indent? 
> [whitespace/indent] [3]
> ERROR: Tools/TestWebKitAPI/Tests/WebKitLegacy/mac/ClosingWebView.mm:93: 
> Weird number of spaces at line-start.  Are you using a 4-space indent? 
> [whitespace/indent] [3]
> ERROR: Tools/TestWebKitAPI/Tests/WebKitLegacy/mac/ClosingWebView.mm:94: 
> Weird number of spaces at line-start.  Are you using a 4-space indent? 
> [whitespace/indent] [3]
> ERROR: Tools/TestWebKitAPI/Tests/WebKitLegacy/mac/ClosingWebView.mm:95: 
> Weird number of spaces at line-start.  Are you using a 4-space indent? 
> [whitespace/indent] [3]
> ERROR: Tools/TestWebKitAPI/Tests/WebKitLegacy/mac/ClosingWebView.mm:96: 
> When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
> ERROR: Tools/TestWebKitAPI/Tests/WebKitLegacy/mac/ClosingWebView.mm:97: 
> Weird number of spaces at line-start.  Are you using a 4-space indent? 
> [whitespace/indent] [3]

These are all to line up the parameters for the creation of the NSEvent object. We do the same indentation in PlatformWebViewMac.mm and CommandBackForward.mm.
Comment 9 Chris Dumez 2018-11-01 16:58:48 PDT
Comment on attachment 353654 [details]
Patch

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

> Tools/TestWebKitAPI/Tests/WebKitLegacy/mac/ClosingWebView.mm:74
> +    RetainPtr<ClosingWebViewThenSendingItAKeyDownEventLoadDelegate> loadDelegate = adoptNS([[ClosingWebViewThenSendingItAKeyDownEventLoadDelegate alloc] init]);

auto
Comment 10 John Wilander 2018-11-01 17:14:11 PDT
Created attachment 353665 [details]
Patch for landing
Comment 11 John Wilander 2018-11-01 17:14:37 PDT
Thanks, Chris!
Comment 12 WebKit Commit Bot 2018-11-01 17:51:21 PDT
Comment on attachment 353665 [details]
Patch for landing

Clearing flags on attachment: 353665

Committed r237711: <https://trac.webkit.org/changeset/237711>
Comment 13 WebKit Commit Bot 2018-11-01 17:51:23 PDT
All reviewed patches have been landed.  Closing bug.