Bug 159856 – [WebKit API] Add SPI to track multiple navigations caused by a single user gesture

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Sam Weinig]

[WebKit API] Add SPI to track multiple navigations caused by a single user gesture

[Sam Weinig]

Created attachment 283851 [details]
Patch

[Sam Weinig]

Putting up for initial discussion / EWS run.  Needs tests and changelogs.

[WebKit Commit Bot]

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.

[Sam Weinig]

Created attachment 283858 [details]
Patch

[WebKit Commit Bot]

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.

[Sam Weinig]

Created attachment 283869 [details]
Patch

[WebKit Commit Bot]

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.

[Sam Weinig]

Created attachment 283871 [details]
Patch

[WebKit Commit Bot]

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 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 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 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.

[Sam Weinig]

(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.

[Sam Weinig]

Committed in https://trac.webkit.org/r203338.

[Csaba Osztrogonác]

(In reply to comment #14)
> Committed in https://trac.webkit.org/r203338.

It broke the Windows build, as the res EWS bubble noticed.

[Csaba Osztrogonác]

(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

[Csaba Osztrogonác]

(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