Bug 159856 - [WebKit API] Add SPI to track multiple navigations caused by a single user gesture
Summary: [WebKit API] Add SPI to track multiple navigations caused by a single user ge...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Sam Weinig
URL:
Keywords:
Depends on: 159875
Blocks:
  Show dependency treegraph
 
Reported: 2016-07-16 12:44 PDT by Sam Weinig
Modified: 2016-07-18 05:38 PDT (History)
6 users (show)

See Also:


Attachments
Patch (90.39 KB, patch)
2016-07-16 12:56 PDT, Sam Weinig
no flags Details | Formatted Diff | Diff
Patch (90.43 KB, patch)
2016-07-16 21:29 PDT, Sam Weinig
no flags Details | Formatted Diff | Diff
Patch (110.74 KB, patch)
2016-07-17 11:27 PDT, Sam Weinig
no flags Details | Formatted Diff | Diff
Patch (116.24 KB, patch)
2016-07-17 13:18 PDT, Sam Weinig
mitz: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Sam Weinig 2016-07-16 12:44:10 PDT
[WebKit API] Add SPI to track multiple navigations caused by a single user gesture
Comment 1 Sam Weinig 2016-07-16 12:56:29 PDT
Created attachment 283851 [details]
Patch
Comment 2 Sam Weinig 2016-07-16 12:57:01 PDT
Putting up for initial discussion / EWS run.  Needs tests and changelogs.
Comment 3 WebKit Commit Bot 2016-07-16 12:58:06 PDT
Attachment 283851 [details] did not pass style-queue:


ERROR: Source/WebKit2/UIProcess/WebProcessProxy.h:99:  'userInitiatedActivity' is incorrectly named. It should be named 'protector' or 'protectedUint64_t'.  [readability/naming/protected] [4]
ERROR: Source/WebKit2/WebProcess/WebProcess.cpp:736:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WebKit2/UIProcess/WebProcessProxy.cpp:675:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WebCore/dom/UserGestureIndicator.h:58:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebCore/dom/UserGestureIndicator.h:70:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 5 in 51 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 4 Sam Weinig 2016-07-16 21:29:37 PDT
Created attachment 283858 [details]
Patch
Comment 5 WebKit Commit Bot 2016-07-16 21:32:24 PDT
Attachment 283858 [details] did not pass style-queue:


ERROR: Source/WebKit2/UIProcess/WebProcessProxy.h:99:  'userInitiatedActivity' is incorrectly named. It should be named 'protector' or 'protectedUint64_t'.  [readability/naming/protected] [4]
ERROR: Source/WebKit2/WebProcess/WebProcess.cpp:736:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WebKit2/UIProcess/WebProcessProxy.cpp:675:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WebCore/dom/UserGestureIndicator.h:58:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebCore/dom/UserGestureIndicator.h:70:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 5 in 51 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 6 Sam Weinig 2016-07-17 11:27:09 PDT
Created attachment 283869 [details]
Patch
Comment 7 WebKit Commit Bot 2016-07-17 11:29:09 PDT
Attachment 283869 [details] did not pass style-queue:


ERROR: Source/WebKit2/UIProcess/API/gtk/WebKitNavigationActionPrivate.h:32:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
ERROR: Source/WebKit2/UIProcess/WebProcessProxy.h:99:  'userInitiatedActivity' is incorrectly named. It should be named 'protector' or 'protectedUint64_t'.  [readability/naming/protected] [4]
ERROR: Source/WebKit2/WebProcess/WebProcess.cpp:734:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WebKit2/UIProcess/WebProcessProxy.cpp:675:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Tools/TestWebKitAPI/Tests/WebKit2Cocoa/UserInitiatedActionInNavigationAction.mm:113:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Tools/TestWebKitAPI/Tests/WebKit2Cocoa/UserInitiatedActionInNavigationAction.mm:122:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
ERROR: Tools/TestWebKitAPI/Tests/WebKit2Cocoa/UserInitiatedActionInNavigationAction.mm:151:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Tools/TestWebKitAPI/Tests/WebKit2Cocoa/UserInitiatedActionInNavigationAction.mm:160:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
ERROR: Tools/TestWebKitAPI/Tests/WebKit2Cocoa/UserInitiatedActionInNavigationAction.mm:189:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Tools/TestWebKitAPI/Tests/WebKit2Cocoa/UserInitiatedActionInNavigationAction.mm:198:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
ERROR: Tools/TestWebKitAPI/Tests/WebKit2Cocoa/UserInitiatedActionInNavigationAction.mm:227:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebCore/dom/UserGestureIndicator.h:58:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebCore/dom/UserGestureIndicator.h:70:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 13 in 55 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 8 Sam Weinig 2016-07-17 13:18:18 PDT
Created attachment 283871 [details]
Patch
Comment 9 WebKit Commit Bot 2016-07-17 13:20:39 PDT
Attachment 283871 [details] did not pass style-queue:


ERROR: Source/WebKit2/UIProcess/API/gtk/WebKitNavigationActionPrivate.h:32:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
ERROR: Source/WebKit2/UIProcess/WebProcessProxy.h:99:  'userInitiatedActivity' is incorrectly named. It should be named 'protector' or 'protectedUint64_t'.  [readability/naming/protected] [4]
ERROR: Source/WebKit2/WebProcess/WebProcess.cpp:734:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WebKit2/UIProcess/WebProcessProxy.cpp:675:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Tools/TestWebKitAPI/Tests/WebKit2Cocoa/UserInitiatedActionInNavigationAction.mm:114:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Tools/TestWebKitAPI/Tests/WebKit2Cocoa/UserInitiatedActionInNavigationAction.mm:123:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
ERROR: Tools/TestWebKitAPI/Tests/WebKit2Cocoa/UserInitiatedActionInNavigationAction.mm:155:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Tools/TestWebKitAPI/Tests/WebKit2Cocoa/UserInitiatedActionInNavigationAction.mm:164:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
ERROR: Tools/TestWebKitAPI/Tests/WebKit2Cocoa/UserInitiatedActionInNavigationAction.mm:196:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Tools/TestWebKitAPI/Tests/WebKit2Cocoa/UserInitiatedActionInNavigationAction.mm:205:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
ERROR: Tools/TestWebKitAPI/Tests/WebKit2Cocoa/UserInitiatedActionInNavigationAction.mm:236:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebCore/dom/UserGestureIndicator.h:58:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebCore/dom/UserGestureIndicator.h:70:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 13 in 57 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 10 mitz 2016-07-17 13:41:53 PDT
Comment on attachment 283871 [details]
Patch

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

> Source/WebCore/ChangeLog:57
> +        (WebCore::NavigationAction::processingUserGesture): Deleted.

This got moved and changed to use the token, but not deleted.

> Source/WebCore/loader/NavigationScheduler.cpp:88
> +    void clearUserGesture() { m_userGestureToForward = nullptr; } // FIXME: Need another value

This FIXME doesn’t seem to apply.
Comment 11 mitz 2016-07-17 13:56:09 PDT
Comment on attachment 283871 [details]
Patch

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

> Source/WebKit2/UIProcess/WebProcessProxy.h:241
> +    UserInitiatedActionMap m_userInitiatedActionMap;

Not a fan of “map” in the name, but it seems to fit with the rest.

> Source/WebKit2/UIProcess/API/APINavigationAction.h:71
> +    bool isProcessingUserGesture() const { return !!m_userInitiatedAction; }

Do we need the !! here?

> Source/WebKit2/UIProcess/API/Cocoa/WKNavigationActionPrivate.h:50
> +@property (nonatomic, readonly) _WKUserInitiatedAction *_userInitiatedAction;

Needs WK_API_AVAILABLE.

> Source/WebKit2/UIProcess/API/gtk/WebKitNavigationActionPrivate.h:32
> +        , isUserGesture(navigationActionData.userGestureTokenIdentifier != 0)

Do we need the explicit != 0 here? Our style is to avoid comparisons to 0.

> Source/WebKit2/WebProcess/WebProcess.cpp:731
> +    if (!token || !token->processingUserGesture())

Why is it important or desirable to return 0 if the token’s is !processingUserGesture()?

> Source/WebKit2/WebProcess/WebProcess.h:401
> +    HashMap<WebCore::UserGestureToken *, uint64_t> m_activeUserGestureTokens;

I’d drop “active” from this name. It doesn’t add much.

> Tools/MiniBrowser/mac/WK2BrowserWindowController.m:393
> -    self.window.title = [NSString stringWithFormat:@"%@%@ [WK2 %d]", _isPrivateBrowsingWindow ? @"ð " : @"", title, _webView._webProcessIdentifier];
> +    self.window.title = [NSString stringWithFormat:@"%@%@ [WK2 %d]", _isPrivateBrowsingWindow ? @"ï " : @"", title, _webView._webProcessIdentifier];

This doesn’t seem to be related.
Comment 12 mitz 2016-07-17 13:56:27 PDT
Comment on attachment 283871 [details]
Patch

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

> Tools/MiniBrowser/mac/WK2BrowserWindowController.m:410
> +    [controller awakeFromNib];

Gross.
Comment 13 Sam Weinig 2016-07-17 15:33:53 PDT
(In reply to comment #11)
> Comment on attachment 283871 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=283871&action=review
> 
> > Source/WebKit2/UIProcess/WebProcessProxy.h:241
> > +    UserInitiatedActionMap m_userInitiatedActionMap;
> 
> Not a fan of “map” in the name, but it seems to fit with the rest.

Going to keep it to match the rest.

> > Source/WebKit2/UIProcess/API/APINavigationAction.h:71
> > +    bool isProcessingUserGesture() const { return !!m_userInitiatedAction; }
> 
> Do we need the !! Here?

Fixed.

> > Source/WebKit2/UIProcess/API/Cocoa/WKNavigationActionPrivate.h:50
> > +@property (nonatomic, readonly) _WKUserInitiatedAction *_userInitiatedAction;
> 
> Needs WK_API_AVAILABLE.

Fixed.

> > Source/WebKit2/UIProcess/API/gtk/WebKitNavigationActionPrivate.h:32
> > +        , isUserGesture(navigationActionData.userGestureTokenIdentifier != 0)
> 
> Do we need the explicit != 0 here? Our style is to avoid comparisons to 0.

Fixed.

> > Source/WebKit2/WebProcess/WebProcess.cpp:731
> > +    if (!token || !token->processingUserGesture())
> 
> Why is it important or desirable to return 0 if the token’s is
> !processingUserGesture()?

We don't want to create a UserInitiatedAction on the UI side if we are only "ProcessingPotentialUserGesture", which can happen, for instance, in the initial stages of a tap.  We only use that state for media.

> 
> > Source/WebKit2/WebProcess/WebProcess.h:401
> > +    HashMap<WebCore::UserGestureToken *, uint64_t> m_activeUserGestureTokens;
> 
> I’d drop “active” from this name. It doesn’t add much.

Fixed.

> > Tools/MiniBrowser/mac/WK2BrowserWindowController.m:393
> > -    self.window.title = [NSString stringWithFormat:@"%@%@ [WK2 %d]", _isPrivateBrowsingWindow ? @"ð " : @"", title, _webView._webProcessIdentifier];
> > +    self.window.title = [NSString stringWithFormat:@"%@%@ [WK2 %d]", _isPrivateBrowsingWindow ? @"ï " : @"", title, _webView._webProcessIdentifier];
> 
> This doesn’t seem to be related.

No idea how that happened. Fixed.
Comment 14 Sam Weinig 2016-07-17 18:08:51 PDT
Committed in https://trac.webkit.org/r203338.
Comment 15 Csaba Osztrogonác 2016-07-18 00:36:45 PDT
(In reply to comment #14)
> Committed in https://trac.webkit.org/r203338.

It broke the Windows build, as the res EWS bubble noticed.
Comment 16 Csaba Osztrogonác 2016-07-18 02:33:26 PDT
(In reply to comment #14)
> Committed in https://trac.webkit.org/r203338.

and it broke the Apple Mac cmake build too - fixed in https://trac.webkit.org/changeset/203343
Comment 17 Csaba Osztrogonác 2016-07-18 05:38:34 PDT
(In reply to comment #15)
> (In reply to comment #14)
> > Committed in https://trac.webkit.org/r203338.
> 
> It broke the Windows build, as the res EWS bubble noticed.

bug report for Windows build fix: bug159875