WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
193919
[GTK] Implement back/forward touchpad gesture
https://bugs.webkit.org/show_bug.cgi?id=193919
Summary
[GTK] Implement back/forward touchpad gesture
Alice Mikhaylenko
Reported
2019-01-28 12:35:05 PST
This is a very nice-to-have feature, and the existing Cocoa/Mac implementation can be ported relatively simply. Patch incoming.
Attachments
Patch
(87.02 KB, patch)
2019-01-28 13:02 PST
,
Alice Mikhaylenko
no flags
Details
Formatted Diff
Diff
Archive of layout-test-results from ews115 for mac-highsierra
(2.02 MB, application/zip)
2019-01-28 14:49 PST
,
EWS Watchlist
no flags
Details
Patch
(134.11 KB, patch)
2019-01-31 07:05 PST
,
Alice Mikhaylenko
no flags
Details
Formatted Diff
Diff
Patch
(134.39 KB, patch)
2019-01-31 07:12 PST
,
Alice Mikhaylenko
no flags
Details
Formatted Diff
Diff
Patch
(135.89 KB, patch)
2019-01-31 07:54 PST
,
Alice Mikhaylenko
no flags
Details
Formatted Diff
Diff
Patch
(135.96 KB, patch)
2019-01-31 08:19 PST
,
Alice Mikhaylenko
no flags
Details
Formatted Diff
Diff
Patch
(157.46 KB, patch)
2019-01-31 11:11 PST
,
Alice Mikhaylenko
no flags
Details
Formatted Diff
Diff
Patch
(162.07 KB, patch)
2019-01-31 12:22 PST
,
Alice Mikhaylenko
no flags
Details
Formatted Diff
Diff
Patch
(162.03 KB, patch)
2019-01-31 12:38 PST
,
Alice Mikhaylenko
no flags
Details
Formatted Diff
Diff
Patch
(162.03 KB, patch)
2019-01-31 13:22 PST
,
Alice Mikhaylenko
no flags
Details
Formatted Diff
Diff
Patch
(162.04 KB, patch)
2019-01-31 13:33 PST
,
Alice Mikhaylenko
no flags
Details
Formatted Diff
Diff
Patch
(162.05 KB, patch)
2019-01-31 13:55 PST
,
Alice Mikhaylenko
no flags
Details
Formatted Diff
Diff
Patch
(163.60 KB, patch)
2019-01-31 14:15 PST
,
Alice Mikhaylenko
no flags
Details
Formatted Diff
Diff
Patch
(163.59 KB, patch)
2019-01-31 14:35 PST
,
Alice Mikhaylenko
no flags
Details
Formatted Diff
Diff
Patch
(163.59 KB, patch)
2019-01-31 14:51 PST
,
Alice Mikhaylenko
no flags
Details
Formatted Diff
Diff
Patch
(164.49 KB, patch)
2019-01-31 15:32 PST
,
Alice Mikhaylenko
no flags
Details
Formatted Diff
Diff
Patch
(166.59 KB, patch)
2019-01-31 16:01 PST
,
Alice Mikhaylenko
no flags
Details
Formatted Diff
Diff
Patch
(163.65 KB, patch)
2019-02-01 00:16 PST
,
Alice Mikhaylenko
no flags
Details
Formatted Diff
Diff
Patch
(165.51 KB, patch)
2019-02-01 00:54 PST
,
Alice Mikhaylenko
no flags
Details
Formatted Diff
Diff
Patch
(165.58 KB, patch)
2019-02-01 04:27 PST
,
Alice Mikhaylenko
no flags
Details
Formatted Diff
Diff
Patch
(98.71 KB, patch)
2019-02-03 13:12 PST
,
Alice Mikhaylenko
no flags
Details
Formatted Diff
Diff
Patch
(98.61 KB, patch)
2019-02-03 13:17 PST
,
Alice Mikhaylenko
no flags
Details
Formatted Diff
Diff
Patch
(165.42 KB, patch)
2019-02-03 13:46 PST
,
Alice Mikhaylenko
no flags
Details
Formatted Diff
Diff
Patch
(165.37 KB, patch)
2019-02-04 03:09 PST
,
Alice Mikhaylenko
no flags
Details
Formatted Diff
Diff
Patch
(171.18 KB, patch)
2019-02-04 08:00 PST
,
Alice Mikhaylenko
no flags
Details
Formatted Diff
Diff
Patch
(171.10 KB, patch)
2019-02-04 10:53 PST
,
Alice Mikhaylenko
no flags
Details
Formatted Diff
Diff
Archive of layout-test-results from ews123 for ios-simulator-wk2
(12.78 MB, application/zip)
2019-02-04 12:42 PST
,
EWS Watchlist
no flags
Details
Patch
(193.60 KB, patch)
2019-02-05 05:53 PST
,
Alice Mikhaylenko
no flags
Details
Formatted Diff
Diff
Patch
(193.65 KB, patch)
2019-02-05 06:15 PST
,
Alice Mikhaylenko
no flags
Details
Formatted Diff
Diff
Patch
(193.91 KB, patch)
2019-02-05 06:46 PST
,
Alice Mikhaylenko
no flags
Details
Formatted Diff
Diff
Patch
(194.78 KB, patch)
2019-02-05 06:57 PST
,
Alice Mikhaylenko
no flags
Details
Formatted Diff
Diff
Archive of layout-test-results from ews104 for mac-highsierra-wk2
(2.70 MB, application/zip)
2019-02-05 08:15 PST
,
EWS Watchlist
no flags
Details
Patch
(194.84 KB, patch)
2019-02-06 04:32 PST
,
Alice Mikhaylenko
no flags
Details
Formatted Diff
Diff
Patch
(193.16 KB, patch)
2019-02-06 04:47 PST
,
Alice Mikhaylenko
no flags
Details
Formatted Diff
Diff
Patch
(205.37 KB, patch)
2019-02-08 09:59 PST
,
Alice Mikhaylenko
no flags
Details
Formatted Diff
Diff
Patch
(205.79 KB, patch)
2019-02-08 11:44 PST
,
Alice Mikhaylenko
no flags
Details
Formatted Diff
Diff
Patch
(208.36 KB, patch)
2019-02-08 12:16 PST
,
Alice Mikhaylenko
no flags
Details
Formatted Diff
Diff
Patch
(208.35 KB, patch)
2019-02-08 12:47 PST
,
Alice Mikhaylenko
no flags
Details
Formatted Diff
Diff
Patch
(207.72 KB, patch)
2019-02-08 13:00 PST
,
Alice Mikhaylenko
no flags
Details
Formatted Diff
Diff
Patch
(205.15 KB, patch)
2019-02-08 13:13 PST
,
Alice Mikhaylenko
no flags
Details
Formatted Diff
Diff
Show Obsolete
(39)
View All
Add attachment
proposed patch, testcase, etc.
Alice Mikhaylenko
Comment 1
2019-01-28 13:02:03 PST
Created
attachment 360363
[details]
Patch
EWS Watchlist
Comment 2
2019-01-28 13:05:06 PST
Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See
http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API
Alice Mikhaylenko
Comment 3
2019-01-28 13:23:08 PST
This is also currently broken with AcceleratedBackingStoreWayland, see
bug 193903
EWS Watchlist
Comment 4
2019-01-28 14:49:30 PST
Comment hidden (obsolete)
Comment on
attachment 360363
[details]
Patch
Attachment 360363
[details]
did not pass mac-debug-ews (mac): Output:
https://webkit-queues.webkit.org/results/10927572
New failing tests: compositing/backing/animate-into-view.html
EWS Watchlist
Comment 5
2019-01-28 14:49:32 PST
Comment hidden (obsolete)
Created
attachment 360378
[details]
Archive of layout-test-results from ews115 for mac-highsierra The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews115 Port: mac-highsierra Platform: Mac OS X 10.13.6
Michael Catanzaro
Comment 6
2019-01-29 16:36:17 PST
Comment on
attachment 360363
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=360363&action=review
I was hoping it could be shared with WPE, but I see the bits in PageClientImpl and WebKitWebViewBase are really GTK-specific. OK then. Carlos Garcia will surely want to review it. And I suppose it's blocked on either
bug #193903
or
bug #193972
. But it looks generally excellent to me. My main concern is the massive code duplication in BackForwardGestureController.cpp and (possibly) ViewSnapshotStore.cpp. At the cost of making it harder to pass EWS and not break Mac, we should be able to share most of this code with the Cocoa classes you copied from, right? ViewGestureController.cpp is already C++, so that shouldn't be too hard to move to cross-platform and merge with BackForwardGestureController.cpp. Then in ViewSnapshotStore.mm... well, maybe there's enough differences there for that to remain platform-specific. I would start by trying to share as much of BackForwardGestureController and ViewGestureController as possible.
> Source/WebKit/UIProcess/API/glib/WebKitSettings.cpp:1461 > + * Enable or disable horizontal swipe gesture for back-forward navigation. > + */
This needs a Since: 2.24 tag. (Of course you can update it to 2.26 if it doesn't make it.)
> Source/WebKit/UIProcess/API/glib/WebKitSettings.cpp:3544 > + * Returns: %TRUE If horizontal swipe gesture will trigger back-forward navigaiton or %FALSE otherwise.
if (lowercase i)
> Source/WebKit/UIProcess/API/glib/WebKitSettings.cpp:3558 > + * @allowed: Value to be set
lowercase v
> Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:504 > +static void allowBackForwardNavigationGesturesChanged(WebKitSettings *settings, GParamSpec*, WebKitWebView* webView)
WebKitSettings* settings
> Source/WebKit/UIProcess/API/gtk/PageClientImpl.h:146 > - void didRestoreScrollPosition() override { } > + void didRestoreScrollPosition() override; > void isPlayingAudioWillChange() final { } > void isPlayingAudioDidChange() final { }
I see this class is already a mix of override and final... you don't have to change this, since it's a preexisting problem and you matched the prevailing style (there were more "overrides" before), but understand of course the class should consistently use one or the other. (We argue about which, but "final" seems to be preferred in WebKit nowadays. There was a recent discussion on webkit-dev, but I don't remember how it ended.) This was a bit of a long review comment to tell you not to change anything, but I just wanted to flag this so you know it's not an example of good code.
> Source/WebKit/UIProcess/API/gtk/WebKitWebViewBase.cpp:1666 > + cairo_t* cr = cairo_create(surface.get());
Surely this is leaked? Shouldn't it be: RefPtr<cairo_t> cr = adoptRef(cairo_create(surface.get()));
> Source/WebKit/UIProcess/gtk/BackForwardGestureController.cpp:230 > + RELEASE_LOG(ViewGestures, "Swipe Snapshot Removal (%0.2f ms) - %{public}s", sinceStart.milliseconds(), log.utf8().data());
Isn't this going to result in a -Wformat warning? %{public} is Mac-specific.
Alice Mikhaylenko
Comment 7
2019-01-30 02:55:20 PST
(In reply to Michael Catanzaro from
comment #6
)
> I was hoping it could be shared with WPE, but I see the bits in > PageClientImpl and WebKitWebViewBase are really GTK-specific. OK then. > Carlos Garcia will surely want to review it. And I suppose it's blocked on > either
bug #193903
or
bug #193972
. But it looks generally excellent to me.
Yes, it relies on GTK heavily for snapshotting and event handling.
> My main concern is the massive code duplication in > BackForwardGestureController.cpp and (possibly) ViewSnapshotStore.cpp. At > the cost of making it harder to pass EWS and not break Mac, we should be > able to share most of this code with the Cocoa classes you copied from, > right? ViewGestureController.cpp is already C++, so that shouldn't be too > hard to move to cross-platform and merge with > BackForwardGestureController.cpp. Then in ViewSnapshotStore.mm... well, > maybe there's enough differences there for that to remain platform-specific. > I would start by trying to share as much of BackForwardGestureController and > ViewGestureController as possible.
Agree, but I have a few questions about implementation that are probably easier discussed in IRC.
> This needs a Since: 2.24 tag. (Of course you can update it to 2.26 if it > doesn't make it.)
Oops.
> > Source/WebKit/UIProcess/API/glib/WebKitSettings.cpp:3544 > > + * Returns: %TRUE If horizontal swipe gesture will trigger back-forward navigaiton or %FALSE otherwise. > > if (lowercase i)
Oops. Also, looking at it now, isn't "if ... will" a grammatical error?
> lowercase v
Oops.
> WebKitSettings* settings
Huh, I assumed the style checking script caught all of these :o
> I see this class is already a mix of override and final... you don't have to > change this, since it's a preexisting problem and you matched the prevailing > style (there were more "overrides" before), but understand of course the > class should consistently use one or the other. (We argue about which, but > "final" seems to be preferred in WebKit nowadays. There was a recent > discussion on webkit-dev, but I don't remember how it ended.) This was a bit > of a long review comment to tell you not to change anything, but I just > wanted to flag this so you know it's not an example of good code.
Ok.
> Surely this is leaked? Shouldn't it be: > > RefPtr<cairo_t> cr = adoptRef(cairo_create(surface.get()));
Oops, it's definitely leaked. Though I get an error with cairo_surface_destroy() in ViewSnapshotStore.cpp being called on already destroyed surface if I do this. Is that one not necessary? I admit I don't completely understand when it gets ref-d/unref-d.
> > Source/WebKit/UIProcess/gtk/BackForwardGestureController.cpp:230 > > + RELEASE_LOG(ViewGestures, "Swipe Snapshot Removal (%0.2f ms) - %{public}s", sinceStart.milliseconds(), log.utf8().data()); > > Isn't this going to result in a -Wformat warning? %{public} is Mac-specific.
Ouch, there was actually a warning there that I was meaning to fix, but forgot. :x Though it's a different warning, and I don't completely understand where it's coming from:
> /home/exalm/Projects/WebKit/Source/WebKit/UIProcess/gtk/BackForwardGestureController.cpp:229:10: warning: variable ‘sinceStart’ set but not used [-Wunused-but-set-variable] > auto sinceStart = MonotonicTime::now() - m_startTime;
I assume RELEASE_LOG() is a macro is substituted with nothing, which would also explain a lack of format warning. What is the preferred way of logging in gtk parts?
Alice Mikhaylenko
Comment 8
2019-01-30 07:37:52 PST
Just for reference, the patch from
bug 193903
fixes the gesture as well :)
Michael Catanzaro
Comment 9
2019-01-30 11:09:01 PST
(In reply to Alexander Mikhaylenko from
comment #7
)
> Oops. Also, looking at it now, isn't "if ... will" a grammatical error?
Nope. Why do you think it would be? I'd put a comma before ", or %FALSE otherwise."
> > WebKitSettings* settings > Huh, I assumed the style checking script caught all of these :o
It normally does; not sure why it missed this one. (In reply to Alexander Mikhaylenko from
comment #7
)
> Oops, it's definitely leaked. Though I get an error with > cairo_surface_destroy() in ViewSnapshotStore.cpp being called on already > destroyed surface if I do this. Is that one not necessary? I admit I don't > completely understand when it gets ref-d/unref-d.
Without investigating the actual refcounting issue here: the RefPtr/GRefPtr is basically "safer g_autoptr" where unref occurs automatically at the bottom of the scope. The main difference is that you have to adopt the initial ref with adoptRef/adoptGRef, otherwise you wind up with an initial refcount of 2.
> Though it's a different warning, and I don't completely understand where > it's coming from: > > /home/exalm/Projects/WebKit/Source/WebKit/UIProcess/gtk/BackForwardGestureController.cpp:229:10: warning: variable ‘sinceStart’ set but not used [-Wunused-but-set-variable] > > auto sinceStart = MonotonicTime::now() - m_startTime; > > I assume RELEASE_LOG() is a macro is substituted with nothing, which would > also explain a lack of format warning. What is the preferred way of logging > in gtk parts?
Hm, you might be building with logging disabled? I think logging is automatically enabled in debug builds, but I'm surprised the release log would not be enabled in release builds. Anyway, the RELEASE_LOG(foo) should ultimately boil down to a fprintf(stderr, foo), but it takes me half an hour every time I investigate all the various layers involved. Maybe you could do something like #if !RELEASE_LOG_DISABLED auto sinceStart = ...; RELEASE_LOG(...); #endif
Adrian Perez
Comment 10
2019-01-30 11:51:07 PST
Just a quick tip, without having the intention of hijacking the review... (In reply to Michael Catanzaro from
comment #9
)
> (In reply to Alexander Mikhaylenko from
comment #7
)
>
> > Oops, it's definitely leaked. Though I get an error with > > cairo_surface_destroy() in ViewSnapshotStore.cpp being called on already > > destroyed surface if I do this. Is that one not necessary? I admit I don't > > completely understand when it gets ref-d/unref-d. > > Without investigating the actual refcounting issue here: the RefPtr/GRefPtr > is basically "safer g_autoptr" where unref occurs automatically at the > bottom of the scope. The main difference is that you have to adopt the > initial ref with adoptRef/adoptGRef, otherwise you wind up with an initial > refcount of 2.
This wiki page is a “hidden” a few clicks away from the front page, and when I found it for the first time, it turned out to be a good read to understand GRefPtr -- it even comes with examples:
https://trac.webkit.org/wiki/GRefPtr
I hope this helps :)
Alice Mikhaylenko
Comment 11
2019-01-31 07:05:25 PST
Created
attachment 360724
[details]
Patch
Alice Mikhaylenko
Comment 12
2019-01-31 07:06:38 PST
It's not finished, I haven't fixed cairo_t leak yet, and ViewSnapshotStore can most probably be shared as well.
Alice Mikhaylenko
Comment 13
2019-01-31 07:12:10 PST
Created
attachment 360725
[details]
Patch
EWS Watchlist
Comment 14
2019-01-31 07:14:09 PST
Comment hidden (obsolete)
Attachment 360725
[details]
did not pass style-queue: WARNING: File exempt from style guide. Skipping: "Source/WebKit/UIProcess/API/gtk/WebKitSettings.h" ERROR: Source/WebKit/UIProcess/ViewGestureController.h:53: _UINavigationInteractiveTransitionBase is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebKit/UIProcess/ViewGestureController.h:54: _UIViewControllerOneToOneTransitionContext is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebKit/UIProcess/ViewGestureController.h:55: _UIViewControllerTransitionContext is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebKit/UIProcess/ViewGestureController.h:132: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit/UIProcess/ViewGestureController.h:344: Extra space before ( in function call [whitespace/parens] [4] Total errors found: 5 in 29 files If any of these errors are false positives, please file a bug against check-webkit-style.
Alice Mikhaylenko
Comment 15
2019-01-31 07:54:13 PST
Created
attachment 360726
[details]
Patch
EWS Watchlist
Comment 16
2019-01-31 07:55:40 PST
Comment hidden (obsolete)
Attachment 360726
[details]
did not pass style-queue: WARNING: File exempt from style guide. Skipping: "Source/WebKit/UIProcess/API/gtk/WebKitSettings.h" ERROR: Source/WebKit/UIProcess/ViewGestureController.h:53: _UINavigationInteractiveTransitionBase is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebKit/UIProcess/ViewGestureController.h:54: _UIViewControllerOneToOneTransitionContext is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebKit/UIProcess/ViewGestureController.h:55: _UIViewControllerTransitionContext is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebKit/UIProcess/ViewGestureController.h:132: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit/UIProcess/ViewGestureController.h:344: Extra space before ( in function call [whitespace/parens] [4] Total errors found: 5 in 30 files If any of these errors are false positives, please file a bug against check-webkit-style.
Alice Mikhaylenko
Comment 17
2019-01-31 08:19:26 PST
Created
attachment 360728
[details]
Patch
EWS Watchlist
Comment 18
2019-01-31 08:21:24 PST
Comment hidden (obsolete)
Attachment 360728
[details]
did not pass style-queue: WARNING: File exempt from style guide. Skipping: "Source/WebKit/UIProcess/API/gtk/WebKitSettings.h" ERROR: Source/WebKit/UIProcess/ViewGestureController.h:53: _UINavigationInteractiveTransitionBase is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebKit/UIProcess/ViewGestureController.h:54: _UIViewControllerOneToOneTransitionContext is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebKit/UIProcess/ViewGestureController.h:55: _UIViewControllerTransitionContext is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebKit/UIProcess/ViewGestureController.h:132: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit/UIProcess/ViewGestureController.h:344: Extra space before ( in function call [whitespace/parens] [4] Total errors found: 5 in 30 files If any of these errors are false positives, please file a bug against check-webkit-style.
Alice Mikhaylenko
Comment 19
2019-01-31 11:11:46 PST
Created
attachment 360745
[details]
Patch
EWS Watchlist
Comment 20
2019-01-31 11:15:37 PST
Comment hidden (obsolete)
Attachment 360745
[details]
did not pass style-queue: WARNING: File exempt from style guide. Skipping: "Source/WebKit/UIProcess/API/gtk/WebKitSettings.h" ERROR: Source/WebKit/UIProcess/ViewGestureController.h:53: _UINavigationInteractiveTransitionBase is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebKit/UIProcess/ViewGestureController.h:54: _UIViewControllerOneToOneTransitionContext is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebKit/UIProcess/ViewGestureController.h:55: _UIViewControllerTransitionContext is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebKit/UIProcess/ViewGestureController.h:132: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit/UIProcess/ViewGestureController.h:344: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit/UIProcess/ViewSnapshotStore.cpp:159: Wrong number of spaces before statement. (expected: 4) [whitespace/indent] [4] ERROR: Source/WebKit/UIProcess/ViewSnapshotStore.cpp:165: Wrong number of spaces before statement. (expected: 4) [whitespace/indent] [4] ERROR: Source/WebKit/UIProcess/ViewSnapshotStore.cpp:165: Wrong number of spaces before statement. (expected: 4) [whitespace/indent] [4] Total errors found: 8 in 30 files If any of these errors are false positives, please file a bug against check-webkit-style.
Alice Mikhaylenko
Comment 21
2019-01-31 12:22:27 PST
Created
attachment 360753
[details]
Patch
EWS Watchlist
Comment 22
2019-01-31 12:26:09 PST
Comment hidden (obsolete)
Attachment 360753
[details]
did not pass style-queue: WARNING: File exempt from style guide. Skipping: "Source/WebKit/UIProcess/API/gtk/WebKitSettings.h" ERROR: Source/WebKit/UIProcess/ViewGestureController.h:53: _UINavigationInteractiveTransitionBase is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebKit/UIProcess/ViewGestureController.h:54: _UIViewControllerOneToOneTransitionContext is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebKit/UIProcess/ViewGestureController.h:55: _UIViewControllerTransitionContext is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebKit/UIProcess/ViewGestureController.h:132: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit/UIProcess/ViewGestureController.h:344: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit/UIProcess/mac/ViewSnapshotStoreMac.mm:75: Wrong number of spaces before statement. (expected: 4) [whitespace/indent] [4] Total errors found: 6 in 32 files If any of these errors are false positives, please file a bug against check-webkit-style.
Alice Mikhaylenko
Comment 23
2019-01-31 12:38:12 PST
Created
attachment 360755
[details]
Patch
EWS Watchlist
Comment 24
2019-01-31 12:41:47 PST
Comment hidden (obsolete)
Attachment 360755
[details]
did not pass style-queue: WARNING: File exempt from style guide. Skipping: "Source/WebKit/UIProcess/API/gtk/WebKitSettings.h" ERROR: Source/WebKit/UIProcess/ViewGestureController.h:53: _UINavigationInteractiveTransitionBase is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebKit/UIProcess/ViewGestureController.h:54: _UIViewControllerOneToOneTransitionContext is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebKit/UIProcess/ViewGestureController.h:55: _UIViewControllerTransitionContext is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebKit/UIProcess/ViewGestureController.h:132: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit/UIProcess/ViewGestureController.h:344: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit/UIProcess/mac/ViewSnapshotStoreMac.mm:73: Wrong number of spaces before statement. (expected: 4) [whitespace/indent] [4] Total errors found: 6 in 32 files If any of these errors are false positives, please file a bug against check-webkit-style.
Alice Mikhaylenko
Comment 25
2019-01-31 13:22:47 PST
Created
attachment 360763
[details]
Patch
EWS Watchlist
Comment 26
2019-01-31 13:25:42 PST
Comment hidden (obsolete)
Attachment 360763
[details]
did not pass style-queue: WARNING: File exempt from style guide. Skipping: "Source/WebKit/UIProcess/API/gtk/WebKitSettings.h" ERROR: Source/WebKit/UIProcess/ViewGestureController.h:53: _UINavigationInteractiveTransitionBase is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebKit/UIProcess/ViewGestureController.h:54: _UIViewControllerOneToOneTransitionContext is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebKit/UIProcess/ViewGestureController.h:55: _UIViewControllerTransitionContext is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebKit/UIProcess/ViewGestureController.h:132: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit/UIProcess/ViewGestureController.h:344: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit/UIProcess/mac/ViewSnapshotStoreMac.mm:73: Wrong number of spaces before statement. (expected: 4) [whitespace/indent] [4] Total errors found: 6 in 32 files If any of these errors are false positives, please file a bug against check-webkit-style.
Alice Mikhaylenko
Comment 27
2019-01-31 13:33:06 PST
Created
attachment 360765
[details]
Patch
EWS Watchlist
Comment 28
2019-01-31 13:35:03 PST
Comment hidden (obsolete)
Attachment 360765
[details]
did not pass style-queue: WARNING: File exempt from style guide. Skipping: "Source/WebKit/UIProcess/API/gtk/WebKitSettings.h" ERROR: Source/WebKit/UIProcess/ViewGestureController.h:53: _UINavigationInteractiveTransitionBase is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebKit/UIProcess/ViewGestureController.h:54: _UIViewControllerOneToOneTransitionContext is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebKit/UIProcess/ViewGestureController.h:55: _UIViewControllerTransitionContext is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebKit/UIProcess/ViewGestureController.h:132: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit/UIProcess/ViewGestureController.h:344: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit/UIProcess/mac/ViewSnapshotStoreMac.mm:73: Wrong number of spaces before statement. (expected: 4) [whitespace/indent] [4] Total errors found: 6 in 32 files If any of these errors are false positives, please file a bug against check-webkit-style.
Alice Mikhaylenko
Comment 29
2019-01-31 13:55:19 PST
Created
attachment 360770
[details]
Patch
EWS Watchlist
Comment 30
2019-01-31 13:57:57 PST
Comment hidden (obsolete)
Attachment 360770
[details]
did not pass style-queue: WARNING: File exempt from style guide. Skipping: "Source/WebKit/UIProcess/API/gtk/WebKitSettings.h" ERROR: Source/WebKit/UIProcess/ViewGestureController.h:53: _UINavigationInteractiveTransitionBase is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebKit/UIProcess/ViewGestureController.h:54: _UIViewControllerOneToOneTransitionContext is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebKit/UIProcess/ViewGestureController.h:55: _UIViewControllerTransitionContext is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebKit/UIProcess/ViewGestureController.h:132: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit/UIProcess/ViewGestureController.h:344: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit/UIProcess/mac/ViewSnapshotStoreMac.mm:73: Wrong number of spaces before statement. (expected: 4) [whitespace/indent] [4] Total errors found: 6 in 32 files If any of these errors are false positives, please file a bug against check-webkit-style.
Alice Mikhaylenko
Comment 31
2019-01-31 14:15:05 PST
Created
attachment 360780
[details]
Patch
EWS Watchlist
Comment 32
2019-01-31 14:19:20 PST
Comment hidden (obsolete)
Attachment 360780
[details]
did not pass style-queue: WARNING: File exempt from style guide. Skipping: "Source/WebKit/UIProcess/API/gtk/WebKitSettings.h" ERROR: Source/WebKit/UIProcess/ViewGestureController.h:53: _UINavigationInteractiveTransitionBase is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebKit/UIProcess/ViewGestureController.h:54: _UIViewControllerOneToOneTransitionContext is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebKit/UIProcess/ViewGestureController.h:55: _UIViewControllerTransitionContext is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebKit/UIProcess/ViewGestureController.h:132: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit/UIProcess/ViewGestureController.h:344: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit/UIProcess/mac/ViewSnapshotStoreMac.mm:73: Wrong number of spaces before statement. (expected: 4) [whitespace/indent] [4] Total errors found: 6 in 32 files If any of these errors are false positives, please file a bug against check-webkit-style.
Alice Mikhaylenko
Comment 33
2019-01-31 14:35:12 PST
Created
attachment 360785
[details]
Patch
EWS Watchlist
Comment 34
2019-01-31 14:38:12 PST
Comment hidden (obsolete)
Attachment 360785
[details]
did not pass style-queue: WARNING: File exempt from style guide. Skipping: "Source/WebKit/UIProcess/API/gtk/WebKitSettings.h" ERROR: Source/WebKit/UIProcess/ViewGestureController.h:53: _UINavigationInteractiveTransitionBase is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebKit/UIProcess/ViewGestureController.h:54: _UIViewControllerOneToOneTransitionContext is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebKit/UIProcess/ViewGestureController.h:55: _UIViewControllerTransitionContext is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebKit/UIProcess/ViewGestureController.h:132: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit/UIProcess/ViewGestureController.h:344: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit/UIProcess/mac/ViewSnapshotStoreMac.mm:73: Wrong number of spaces before statement. (expected: 4) [whitespace/indent] [4] Total errors found: 6 in 32 files If any of these errors are false positives, please file a bug against check-webkit-style.
Alice Mikhaylenko
Comment 35
2019-01-31 14:51:57 PST
Created
attachment 360789
[details]
Patch
EWS Watchlist
Comment 36
2019-01-31 14:54:51 PST
Comment hidden (obsolete)
Attachment 360789
[details]
did not pass style-queue: WARNING: File exempt from style guide. Skipping: "Source/WebKit/UIProcess/API/gtk/WebKitSettings.h" ERROR: Source/WebKit/UIProcess/ViewGestureController.h:53: _UINavigationInteractiveTransitionBase is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebKit/UIProcess/ViewGestureController.h:54: _UIViewControllerOneToOneTransitionContext is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebKit/UIProcess/ViewGestureController.h:55: _UIViewControllerTransitionContext is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebKit/UIProcess/ViewGestureController.h:132: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit/UIProcess/ViewGestureController.h:344: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit/UIProcess/mac/ViewSnapshotStoreMac.mm:73: Wrong number of spaces before statement. (expected: 4) [whitespace/indent] [4] Total errors found: 6 in 32 files If any of these errors are false positives, please file a bug against check-webkit-style.
Alice Mikhaylenko
Comment 37
2019-01-31 15:32:47 PST
Created
attachment 360795
[details]
Patch
EWS Watchlist
Comment 38
2019-01-31 15:36:15 PST
Comment hidden (obsolete)
Attachment 360795
[details]
did not pass style-queue: WARNING: File exempt from style guide. Skipping: "Source/WebKit/UIProcess/API/gtk/WebKitSettings.h" ERROR: Source/WebKit/UIProcess/ViewGestureController.h:53: _UINavigationInteractiveTransitionBase is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebKit/UIProcess/ViewGestureController.h:54: _UIViewControllerOneToOneTransitionContext is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebKit/UIProcess/ViewGestureController.h:55: _UIViewControllerTransitionContext is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebKit/UIProcess/ViewGestureController.h:132: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit/UIProcess/ViewGestureController.h:344: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit/UIProcess/mac/ViewSnapshotStoreMac.mm:73: Wrong number of spaces before statement. (expected: 4) [whitespace/indent] [4] Total errors found: 6 in 33 files If any of these errors are false positives, please file a bug against check-webkit-style.
Alice Mikhaylenko
Comment 39
2019-01-31 16:01:59 PST
Created
attachment 360800
[details]
Patch
EWS Watchlist
Comment 40
2019-01-31 16:06:06 PST
Comment hidden (obsolete)
Attachment 360800
[details]
did not pass style-queue: WARNING: File exempt from style guide. Skipping: "Source/WebKit/UIProcess/API/gtk/WebKitSettings.h" ERROR: Source/WebKit/UIProcess/ViewGestureController.h:53: _UINavigationInteractiveTransitionBase is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebKit/UIProcess/ViewGestureController.h:54: _UIViewControllerOneToOneTransitionContext is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebKit/UIProcess/ViewGestureController.h:55: _UIViewControllerTransitionContext is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebKit/UIProcess/ViewGestureController.h:132: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit/UIProcess/ViewGestureController.h:344: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit/UIProcess/mac/ViewSnapshotStoreMac.mm:73: Wrong number of spaces before statement. (expected: 4) [whitespace/indent] [4] Total errors found: 6 in 33 files If any of these errors are false positives, please file a bug against check-webkit-style.
Michael Catanzaro
Comment 41
2019-01-31 17:47:26 PST
Comment on
attachment 360800
[details]
Patch Careful, you've duplicated your changelog in Tools/ChangeLog
Alice Mikhaylenko
Comment 42
2019-02-01 00:16:21 PST
Created
attachment 360846
[details]
Patch
EWS Watchlist
Comment 43
2019-02-01 00:18:17 PST
Comment hidden (obsolete)
Attachment 360846
[details]
did not pass style-queue: WARNING: File exempt from style guide. Skipping: "Source/WebKit/UIProcess/API/gtk/WebKitSettings.h" ERROR: Source/WebKit/UIProcess/ViewGestureController.h:53: _UINavigationInteractiveTransitionBase is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebKit/UIProcess/ViewGestureController.h:54: _UIViewControllerOneToOneTransitionContext is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebKit/UIProcess/ViewGestureController.h:55: _UIViewControllerTransitionContext is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebKit/UIProcess/ViewGestureController.h:132: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit/UIProcess/ViewGestureController.h:344: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit/UIProcess/mac/ViewSnapshotStoreMac.mm:73: Wrong number of spaces before statement. (expected: 4) [whitespace/indent] [4] Total errors found: 6 in 32 files If any of these errors are false positives, please file a bug against check-webkit-style.
Alice Mikhaylenko
Comment 44
2019-02-01 00:23:11 PST
> Careful, you've duplicated your changelog in Tools/ChangeLog
Oops. I admit I'm not checking the changelog until it builds. :/
Alice Mikhaylenko
Comment 45
2019-02-01 00:54:02 PST
Created
attachment 360848
[details]
Patch
EWS Watchlist
Comment 46
2019-02-01 00:56:23 PST
Comment hidden (obsolete)
Attachment 360848
[details]
did not pass style-queue: WARNING: File exempt from style guide. Skipping: "Source/WebKit/UIProcess/API/gtk/WebKitSettings.h" ERROR: Source/WebKit/UIProcess/ViewGestureController.h:53: _UINavigationInteractiveTransitionBase is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebKit/UIProcess/ViewGestureController.h:54: _UIViewControllerOneToOneTransitionContext is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebKit/UIProcess/ViewGestureController.h:55: _UIViewControllerTransitionContext is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebKit/UIProcess/ViewGestureController.h:132: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit/UIProcess/ViewGestureController.h:344: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit/UIProcess/mac/ViewSnapshotStoreMac.mm:73: Wrong number of spaces before statement. (expected: 4) [whitespace/indent] [4] Total errors found: 6 in 32 files If any of these errors are false positives, please file a bug against check-webkit-style.
Alice Mikhaylenko
Comment 47
2019-02-01 04:27:06 PST
Created
attachment 360858
[details]
Patch
Alice Mikhaylenko
Comment 48
2019-02-01 04:28:21 PST
Ok, this one should be good (aside from the code style script needing an update)
EWS Watchlist
Comment 49
2019-02-01 04:30:13 PST
Comment hidden (obsolete)
Attachment 360858
[details]
did not pass style-queue: WARNING: File exempt from style guide. Skipping: "Source/WebKit/UIProcess/API/gtk/WebKitSettings.h" ERROR: Source/WebKit/UIProcess/ViewGestureController.h:53: _UINavigationInteractiveTransitionBase is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebKit/UIProcess/ViewGestureController.h:54: _UIViewControllerOneToOneTransitionContext is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebKit/UIProcess/ViewGestureController.h:55: _UIViewControllerTransitionContext is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebKit/UIProcess/ViewGestureController.h:132: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit/UIProcess/ViewGestureController.h:344: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit/UIProcess/mac/ViewSnapshotStoreMac.mm:73: Wrong number of spaces before statement. (expected: 4) [whitespace/indent] [4] Total errors found: 6 in 32 files If any of these errors are false positives, please file a bug against check-webkit-style.
Michael Catanzaro
Comment 50
2019-02-02 15:01:15 PST
Alex, who'd be a good Apple reviewer for this? This is another big patch like we have in
bug #174816
.
Alice Mikhaylenko
Comment 51
2019-02-03 13:12:02 PST
Created
attachment 361024
[details]
Patch
Alice Mikhaylenko
Comment 52
2019-02-03 13:14:26 PST
I've rebased the patch and removed Wayland check. There's a bug where we don't get any events when user lifts fingers from touchpad (so the gesture becomes stuck mid-progress); I assumed it always happens on X11, but turns out it only happens in XWayland in a Wayland session.
Alice Mikhaylenko
Comment 53
2019-02-03 13:17:13 PST
Created
attachment 361026
[details]
Patch
Alice Mikhaylenko
Comment 54
2019-02-03 13:18:20 PST
Sorry for the churn, forgot to update changelog for the Wayland change :x
Michael Catanzaro
Comment 55
2019-02-03 13:24:03 PST
Comment on
attachment 360858
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=360858&action=review
Really good work getting this to build and pass tests for Mac without a Mac. Hopefully the gesture still works there. Would be amazing if someone from Apple could check; otherwise, I'm sure it will be noticed quickly if it breaks. This passes my review, but it needs to be approved by an owner too. I assume the goal is for there to be no functional changes on Apple platforms, right? That should make reviewing much easier.
> Source/WebKit/ChangeLog:21 > + This is only enabled on Wayland, since on X11 we don't get any events when > + the gesture ends. Also, it's only allowed for touchpads, even though it can > + theoretically work with touch mice and trackpoints.
I know you discovered it doesn't need to be limited to Wayland; will you update this patch again?
> Source/WebKit/SourcesCocoa.txt:207 > +UIProcess/ViewGestureController.cpp > +UIProcess/ViewSnapshotStore.cpp @no-unify
And you'll report a new bug to un-unify it?
> Source/WebKit/SourcesGTK.txt:116 > +UIProcess/ViewGestureController.cpp @no-unify > +UIProcess/ViewSnapshotStore.cpp
For SourcesCocoa.txt you build ViewGestureController.cpp in unified build and ViewSnapshotStore.cpp with @no-unify; here you do the opposite. Let's match what you've done for Cocoa?
> Source/WebKit/UIProcess/API/glib/WebKitSettings.cpp:85 > +#if PLATFORM(GTK) > + bool allowBackForwardNavigationGestures { false }; > +#endif
Any reason you chose for it to be disabled by default? It means very few apps will enable it.
> Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:510 > + ViewGestureController* controller = &webkitWebViewBaseViewGestureController(WEBKIT_WEB_VIEW_BASE(webView)); > + > + if (controller) > + controller->setSwipeGestureEnabled(allowsBackForwardNavigationGestures);
webkitWebViewBaseViewGestureController returns a reference, which is guaranteed to be non-null (or it's a programmer error if it somehow is), so converting it to a pointer and then checking if it's null doesn't make sense. Just assume it's nonnull and use it. If that's ever wrong, you have a bug elsewhere.
> Source/WebKit/UIProcess/API/gtk/PageClientImpl.cpp:71 > + WebPageProxy* pageProxy = webkitWebViewBaseGetPage(WEBKIT_WEB_VIEW_BASE(m_viewWidget));
Do we need to null-check pageProxy? If not, add an ASSERT(pageProxy), please. Since webkitWebViewBaseGetPage returns a pointer and not a reference, we should assume it can be nullptr unless proven otherwise. I almost asked you to do this in a bunch of places in WebKitWebViewBase.cpp as well, but that file has a pervasive assumption that it is nonnull. Shame it doesn't look easy to convert from RefPtr to Ref (like RefPtr, but can't be nullptr).
> Source/WebKit/UIProcess/API/gtk/PageClientImpl.cpp:424 > + ViewGestureController& controller = webkitWebViewBaseViewGestureController(WEBKIT_WEB_VIEW_BASE(m_viewWidget)); > + > + if (controller.isSwipeGestureEnabled()) {
Style nit: we'd normally avoid the blank line here, since grabbing controller is just setup for the conditional.
> Source/WebKit/UIProcess/API/gtk/WebKitWebViewBase.cpp:206 > #if HAVE(GTK_GESTURES) > std::unique_ptr<GestureController> gestureController; > #endif > + std::unique_ptr<ViewGestureController> viewGestureController;
I guess it doesn't use GTK+ gestures?
> Source/WebKit/UIProcess/API/gtk/WebKitWebViewBase.cpp:554 > + cairo_pattern_t* group = cairo_pop_group(cr); > + controller.draw(cr, group); > + cairo_pattern_destroy(group);
Can you use a RefPtr here? We try to avoid holding ownership in raw pointers whenever possible. #include <WebCore/RefPtrCairo.h> RefPtr<cairo_pattern_t> group = adoptRef(cairo_pop_group(cr)); controller.draw(cr, group.get()); Even if you were about transfer ownership immediately thereafter, we like to adopt it in a RefPtr and then call leakRef() to make the transfer explicit.
> Source/WebKit/UIProcess/API/gtk/WebKitWebViewBase.cpp:1679 > + return WTFMove(snapshot);
Never move into a return statement: it breaks return value optimization. And the function returns a RefPtr<WebKit::ViewSnapshot>, not a RefPtr<WebKit::ViewSnapshot>&&. Better simply: return snapshot; You can also remove the extra blank lines here and above. We don't normally leave a blank line between declaring a variable and using it.
> Source/WebKit/UIProcess/ViewGestureController.cpp:161 > + if (auto loadCallback = WTFMove(m_loadCallback)) > + loadCallback();
I know this is a problem with the existing code, but this is no good. We should be explicit in resetting m_loadCallback, rather than just trusting the move constructor will reset it to nullptr for us. (Which I'm sure it does, otherwise this would not work, but it's just good practice.) if (auto loadCallback = std::exchange(m_loadCallback, nullptr)) loadCallback();
> Source/WebKit/UIProcess/ViewGestureController.h:258 > + CALayer *determineSnapshotLayerParent() const; > + CALayer *determineLayerAdjacentToSnapshotForParent(SwipeDirection, CALayer *snapshotLayerParent) const;
CALayer*
> Source/WebKit/UIProcess/ViewGestureController.h:346 > + UIView *m_liveSwipeView { nullptr };
UIView*
> Source/WebKit/UIProcess/ViewSnapshotStore.cpp:53 > +ViewSnapshotStore& ViewSnapshotStore::singleton() > +{ > + static ViewSnapshotStore& store = *new ViewSnapshotStore; > + return store;
Go ahead and change it to use NeverDestroyed<ViewSnapshotStore> here. Of course, better to not have singletons at all, but that's a preexisting problem and this isn't the right patch to try solving it.
> Source/WebKit/UIProcess/gtk/ViewGestureControllerGtk.cpp:66 > + GdkDevice* device = gdk_event_get_source_device((GdkEvent*) event);
No C-style casts allowed! static_cast<GdkEvent*>(event);
> Source/WebKit/UIProcess/gtk/ViewGestureControllerGtk.cpp:201 > + m_swipeProgressTracker.startTracking(targetItem, direction);
WTFMove(targetItem),
> Source/WebKit/UIProcess/gtk/ViewGestureControllerGtk.cpp:218 > +void ViewGestureController::SwipeProgressTracker::startTracking(RefPtr<WebBackForwardListItem> targetItem, SwipeDirection direction)
RefPtr<WebBackForwardListItem>&& targetItem,
> Source/WebKit/UIProcess/gtk/ViewGestureControllerGtk.cpp:220 > + if (m_state != State::None)
Extra space here
> Source/WebKit/UIProcess/gtk/ViewGestureControllerGtk.cpp:250 > +bool ViewGestureController::SwipeProgressTracker::handleEvent(GdkEventScroll *event)
GdkEventScroll* event
> Source/WebKit/UIProcess/gtk/ViewGestureControllerGtk.cpp:363 > +void ViewGestureController::beginSwipeGesture(WebBackForwardListItem* targetItem, SwipeDirection direction) > +{ > + m_webPageProxy.navigationGestureDidBegin(); > + > + willBeginGesture(ViewGestureType::Swipe); > + > + ViewSnapshot* snapshot = targetItem->snapshot();
Since you're assuming it's nonnull, this function should take a WebBackForwardListItem&, not a WebBackForwardListItem*. Or if it can be nullptr, better null-check it. Or, if it needs to be a pointer to match Apple's implementation, you can ASSERT(targetItem).
> Source/WebKit/UIProcess/gtk/ViewGestureControllerGtk.cpp:386 > +void ViewGestureController::handleSwipeGesture(WebBackForwardListItem* targetItem, double progress, SwipeDirection direction) > +{ > + gtk_widget_queue_draw(m_webPageProxy.viewWidget()); > +}
I guess we have unused variable warnings turned off for Source/WebKit, but you should still omit the parameter names if they're not used.
> Source/WebKit/UIProcess/gtk/ViewGestureControllerGtk.cpp:397 > +void ViewGestureController::endSwipeGesture(WebBackForwardListItem* targetItem, bool cancelled) > +{ > + if (cancelled) { > + removeSwipeSnapshot(); > + m_webPageProxy.navigationGestureDidEnd(false, *targetItem);
Again: since you're assuming it's nonnull, this function should take a WebBackForwardListItem&, not a WebBackForwardListItem*. Or if it can be nullptr, better null-check it. Or, if it needs to be a pointer to match Apple's implementation, you can ASSERT(targetItem).
> Source/WebKit/UIProcess/gtk/ViewGestureControllerGtk.cpp:419 > + ViewSnapshot* snapshot = targetItem->snapshot(); > + if (snapshot) > + m_backgroundColorForCurrentSnapshot = snapshot->backgroundColor();
if (ViewSnapshot* snapshot = targetItem->snapshot()) ...;
> Source/WebKit/UIProcess/gtk/ViewGestureControllerGtk.cpp:454 > +void ViewGestureController::draw(cairo_t* cr, cairo_pattern_t* pageGroup)
This is one of our few exceptions to our rule to use references rather than pointers. We actually do use pointers for GObjects and other C API types. I don't think it makes sense, since it gives up the safety of references, but anyway, that's what we do. So no need to change anything here.
> Source/WebKit/UIProcess/gtk/ViewGestureControllerGtk.cpp:473 > + cairo_pattern_t* shadowPattern = cairo_pattern_create_linear(0, 0, -swipeOverlayShadowWidth, 0);
RefPtr<cairo_pattern_t> shadowPattern = adoptRef(cairo_pattern_create_linear(...)); Then you change direct use of shadowPattern to shadowPattern.get(), and kill the call to cairo_pattern_destroy.
> Source/WebKit/UIProcess/gtk/ViewGestureControllerGtk.cpp:537 > +bool ViewGestureController::beginSimulatedSwipeInDirectionForTesting(SwipeDirection) > +{ > + notImplemented(); > + return false; > +} > + > +bool ViewGestureController::completeSimulatedSwipeInDirectionForTesting(SwipeDirection) > +{ > + notImplemented(); > + return false; > +}
Don't actually call notImplemented() unless it's a TODO to implement it in the future. It creates a spammy debug warning. Better just return false.
> Source/WebKit/UIProcess/gtk/ViewSnapshotStoreGtk.cpp:36 > +Ref<ViewSnapshot> ViewSnapshot::create(RefPtr<cairo_surface_t> surface) > +{ > + return adoptRef(*new ViewSnapshot(WTFMove(surface)));
RefPtr<cairo_surface_t>&& surface
> Source/WebKit/UIProcess/gtk/ViewSnapshotStoreGtk.cpp:40 > +ViewSnapshot::ViewSnapshot(RefPtr<cairo_surface_t> surface) > + : m_surface(WTFMove(surface))
RefPtr<cairo_surface_t>&& surface
Michael Catanzaro
Comment 56
2019-02-03 13:24:30 PST
Comment on
attachment 360858
[details]
Patch I actually reviewed the older version of your patch. I'll assume you didn't make it worse. ;)
Alice Mikhaylenko
Comment 57
2019-02-03 13:46:46 PST
Created
attachment 361027
[details]
Patch
EWS Watchlist
Comment 58
2019-02-03 13:50:02 PST
Comment hidden (obsolete)
Attachment 361027
[details]
did not pass style-queue: WARNING: File exempt from style guide. Skipping: "Source/WebKit/UIProcess/API/gtk/WebKitSettings.h" ERROR: Source/WebKit/UIProcess/ViewGestureController.cpp:40: Alphabetical sorting problem. [build/include_order] [4] ERROR: Source/WebKit/UIProcess/ViewGestureController.h:53: _UINavigationInteractiveTransitionBase is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebKit/UIProcess/ViewGestureController.h:54: _UIViewControllerOneToOneTransitionContext is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebKit/UIProcess/ViewGestureController.h:55: _UIViewControllerTransitionContext is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebKit/UIProcess/ViewGestureController.h:132: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit/UIProcess/ViewGestureController.h:344: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit/UIProcess/mac/ViewSnapshotStoreMac.mm:73: Wrong number of spaces before statement. (expected: 4) [whitespace/indent] [4] Total errors found: 7 in 32 files If any of these errors are false positives, please file a bug against check-webkit-style.
Alice Mikhaylenko
Comment 59
2019-02-03 14:04:43 PST
The last patch doesn't address the comments from
comment 55
, it's a fix for the last 2 borked patches where I forgot to add new files before uploading. ---
> I know you discovered it doesn't need to be limited to Wayland; will you update this patch again?
Yes, already done in the later versions.
> And you'll report a new bug to un-unify it?
Ok, when it's in :)
> For SourcesCocoa.txt you build ViewGestureController.cpp in unified build and ViewSnapshotStore.cpp with @no-unify; here you do the opposite. Let's match what you've done for Cocoa?
Sure. That was an attempt to get it to build somehow :D
> Any reason you chose for it to be disabled by default? It means very few apps will enable it.
It's disabled by default on Mac, and also this way we don't change behavior for the existing apps, like in auth widgets, like the Firefox Sync one in Epiphany preferences. I can enable it by default if you'd prefer that.
> I guess it doesn't use GTK+ gestures?
It doesn't, it uses scroll events. That's the only reason it works in X11.
> Don't actually call notImplemented() unless it's a TODO to implement it in the future. It creates a spammy debug warning. Better just return false.
This was in the equivalent Mac code, but ok. The rest of comments: ok
Alice Mikhaylenko
Comment 60
2019-02-04 03:09:25 PST
Created
attachment 361050
[details]
Patch
EWS Watchlist
Comment 61
2019-02-04 03:12:27 PST
Comment hidden (obsolete)
Attachment 361050
[details]
did not pass style-queue: WARNING: File exempt from style guide. Skipping: "Source/WebKit/UIProcess/API/gtk/WebKitSettings.h" ERROR: Source/WebKit/UIProcess/ViewGestureController.h:53: _UINavigationInteractiveTransitionBase is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebKit/UIProcess/ViewGestureController.h:54: _UIViewControllerOneToOneTransitionContext is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebKit/UIProcess/ViewGestureController.h:55: _UIViewControllerTransitionContext is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebKit/UIProcess/ViewGestureController.h:132: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit/UIProcess/ViewGestureController.h:344: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit/UIProcess/mac/ViewSnapshotStoreMac.mm:73: Wrong number of spaces before statement. (expected: 4) [whitespace/indent] [4] Total errors found: 6 in 32 files If any of these errors are false positives, please file a bug against check-webkit-style.
Michael Catanzaro
Comment 62
2019-02-04 06:52:24 PST
r=me, but it needs two more reviews: * Since it adds new GTK API, it needs to be approved by another GTK reviewer. Normally that would be Carlos Garcia. * Since it modifies cross-platform and Apple code under Source/WebKit, it needs to be approved by an owner
Alice Mikhaylenko
Comment 63
2019-02-04 08:00:19 PST
Created
attachment 361056
[details]
Patch
EWS Watchlist
Comment 64
2019-02-04 08:04:25 PST
Comment hidden (obsolete)
Attachment 361056
[details]
did not pass style-queue: WARNING: File exempt from style guide. Skipping: "Source/WebKit/UIProcess/API/gtk/WebKitSettings.h" ERROR: Source/WebKit/UIProcess/ViewGestureController.h:53: _UINavigationInteractiveTransitionBase is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebKit/UIProcess/ViewGestureController.h:54: _UIViewControllerOneToOneTransitionContext is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebKit/UIProcess/ViewGestureController.h:55: _UIViewControllerTransitionContext is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebKit/UIProcess/ViewGestureController.h:132: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit/UIProcess/ViewGestureController.h:344: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit/UIProcess/mac/ViewSnapshotStoreMac.mm:73: Wrong number of spaces before statement. (expected: 4) [whitespace/indent] [4] Total errors found: 6 in 35 files If any of these errors are false positives, please file a bug against check-webkit-style.
Tim Horton
Comment 65
2019-02-04 09:37:05 PST
(In reply to Michael Catanzaro from
comment #62
)
> r=me, but it needs two more reviews: > > * Since it adds new GTK API, it needs to be approved by another GTK > reviewer. Normally that would be Carlos Garcia. > * Since it modifies cross-platform and Apple code under Source/WebKit, it > needs to be approved by an owner
Also I wrote 90% of this in the first place so I’d love to look over it too :P Cc’ing myself to look in a bit
Alice Mikhaylenko
Comment 66
2019-02-04 10:53:42 PST
Created
attachment 361074
[details]
Patch
Alice Mikhaylenko
Comment 67
2019-02-04 10:56:36 PST
Another reupload :) Fixed a bug I've probably introduced during merging the changes back into Mac implementation. (At least I remember already doing this change before, should reset SwipeProgressTracker state after calling m_webPageProxy.navigationGestureSnapshotWasRemoved(), not before, otherwise there's a rare flickering)
EWS Watchlist
Comment 68
2019-02-04 10:57:25 PST
Comment hidden (obsolete)
Attachment 361074
[details]
did not pass style-queue: WARNING: File exempt from style guide. Skipping: "Source/WebKit/UIProcess/API/gtk/WebKitSettings.h" ERROR: Source/WebKit/UIProcess/ViewGestureController.h:53: _UINavigationInteractiveTransitionBase is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebKit/UIProcess/ViewGestureController.h:54: _UIViewControllerOneToOneTransitionContext is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebKit/UIProcess/ViewGestureController.h:55: _UIViewControllerTransitionContext is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebKit/UIProcess/ViewGestureController.h:132: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit/UIProcess/ViewGestureController.h:344: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit/UIProcess/mac/ViewSnapshotStoreMac.mm:73: Wrong number of spaces before statement. (expected: 4) [whitespace/indent] [4] Total errors found: 6 in 35 files If any of these errors are false positives, please file a bug against check-webkit-style.
EWS Watchlist
Comment 69
2019-02-04 12:42:22 PST
Comment hidden (obsolete)
Comment on
attachment 361074
[details]
Patch
Attachment 361074
[details]
did not pass ios-sim-ews (ios-simulator-wk2): Output:
https://webkit-queues.webkit.org/results/11027698
New failing tests: imported/w3c/web-platform-tests/webrtc/simplecall-no-ssrcs.https.html
EWS Watchlist
Comment 70
2019-02-04 12:42:25 PST
Comment hidden (obsolete)
Created
attachment 361084
[details]
Archive of layout-test-results from ews123 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews123 Port: ios-simulator-wk2 Platform: Mac OS X 10.13.6
Michael Catanzaro
Comment 71
2019-02-04 12:46:11 PST
Don't worry about the test failure. Pretty sure you didn't break WebRTC. Tests are flaky sometimes.
Tim Horton
Comment 72
2019-02-04 13:03:46 PST
Comment on
attachment 361074
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=361074&action=review
> Source/WebKit/Shared/SessionState.h:28 > +#if PLATFORM(COCOA) || PLATFORM(GTK)
Now that it's not in /Cocoa you can just include it unconditionally
> Source/WebKit/UIProcess/ViewGestureController.h:32 > +#if PLATFORM(GTK) > +#include <WebCore/CairoUtilities.h> > +#endif
Conditional includes should be in their own paragraph down below
> Source/WebKit/UIProcess/ViewGestureController.h:170 > + void draw(cairo_t*, cairo_pattern_t*);
Probably stick this before the `// Testing` section
> Source/WebKit/UIProcess/ViewSnapshotStore.h:69 > +#else
Shouldn't most of these #else's be #elif PLATFORM(GTK) to make it easier to add more platforms later?
> Source/WebKit/UIProcess/gtk/ViewGestureControllerGtk.cpp:46 > +static const double swipeOverlayShadowGradientOffsets[] = { 0, 0.03125, 0.0625, 0.0938, 0.125, 0.1875, 0.25, 0.375, 0.4375, 0.5, 0.5625, 0.625, 0.6875, 0.75, 0.875, 1. }; > +static const double swipeOverlayShadowGradientAlpha[] = { 1, 0.99, 0.98, 0.95, 0.92, 0.82, 0.71, 0.46, 0.35, 0.25, 0.17, 0.11, 0.07, 0.04, 0.01, 0. };
Do you really want the modern-macOS-style gradients? That is surprising!
> Source/WebKit/UIProcess/gtk/ViewGestureControllerGtk.cpp:77 > +bool ViewGestureController::PendingSwipeTracker::scrollEventCanBecomeSwipe(GdkEventScroll *event, ViewGestureController::SwipeDirection& potentialSwipeDirection)
I wonder if we can factor some more of this out into shared code (a variant of this that just takes deltas, which scrollEventCanBecomeSwipe calls after extracting the deltas from the event, for example)
> Source/WebKit/UIProcess/gtk/ViewGestureControllerGtk.cpp:105 > +bool ViewGestureController::PendingSwipeTracker::handleEvent(GdkEventScroll *event)
Ditto
Alice Mikhaylenko
Comment 73
2019-02-04 13:39:05 PST
> Now that it's not in /Cocoa you can just include it unconditionally
Ok
> Shouldn't most of these #else's be #elif PLATFORM(GTK) to make it easier to add more platforms later?
It should.
> Do you really want the modern-macOS-style gradients? That is surprising!
Sorry :p Just ported what there was, and nobody minded, so I didn't change it. :)
> I wonder if we can factor some more of this out into shared code (a variant of this that just takes deltas, which scrollEventCanBecomeSwipe calls after extracting the deltas from the event, for example)
Hmm, yes. It also uses phase in macOS code, and is_stop in GTK code (and a device type check, but I should probably remove that one anyway), so should be doable. Not sure about handleEvent(), since it calls tryToStartSwipe(), which in turn calls trackSwipeEventWithOptions on macOS, so we still need the original event, but can probably factor out the checks.
Alice Mikhaylenko
Comment 74
2019-02-05 05:53:13 PST
Created
attachment 361184
[details]
Patch
EWS Watchlist
Comment 75
2019-02-05 05:56:36 PST
Comment hidden (obsolete)
Attachment 361184
[details]
did not pass style-queue: WARNING: File exempt from style guide. Skipping: "Source/WebKit/UIProcess/API/gtk/WebKitSettings.h" ERROR: Source/WebKit/UIProcess/ViewGestureController.h:53: _UINavigationInteractiveTransitionBase is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebKit/UIProcess/ViewGestureController.h:54: _UIViewControllerOneToOneTransitionContext is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebKit/UIProcess/ViewGestureController.h:55: _UIViewControllerTransitionContext is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebKit/UIProcess/ViewGestureController.h:132: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit/UIProcess/ViewGestureController.h:350: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit/UIProcess/mac/ViewSnapshotStoreMac.mm:73: Wrong number of spaces before statement. (expected: 4) [whitespace/indent] [4] Total errors found: 6 in 41 files If any of these errors are false positives, please file a bug against check-webkit-style.
Alice Mikhaylenko
Comment 76
2019-02-05 06:02:28 PST
Addressed comments from
comment 72
. I've moved pretty much all shared code (except beginSwipeGesture(), removeSwipeSnapshot() and platformTeardown(), there are just a couple shared lines there, and major differences otherwise) to ViewGestureController.cpp. As said, the original events are still needed in some places, so I've added the following functions: bool scrollEventCanStartSwipe(PlatformScrollEvent); bool scrollEventCanEndSwipe(PlatformScrollEvent); WebCore::FloatSize scrollEventGetScrollingDeltas(PlatformScrollEvent); that are implemented in Mac- and GTK-specific files, and made cross-platform logic uses these for accessing any properties and PlatformScrollEvent in signatures. Also while merging them, noticed that render tree size threshold logic, that I initially assumed was Mac-specific, should be used in GTK as well, so that part is now also shared, and there are a few changes in WebProcess/Page/ (just re-shuffling `#if`s) to enable that. As always, I don't know if the above patch builds on Mac, EWS will tell :)
> Do you really want the modern-macOS-style gradients? That is surprising!
I've asked GNOME designers about it, they said it looks fine :)
Alice Mikhaylenko
Comment 77
2019-02-05 06:15:55 PST
Created
attachment 361185
[details]
Patch
EWS Watchlist
Comment 78
2019-02-05 06:19:31 PST
Comment hidden (obsolete)
Attachment 361185
[details]
did not pass style-queue: WARNING: File exempt from style guide. Skipping: "Source/WebKit/UIProcess/API/gtk/WebKitSettings.h" ERROR: Source/WebKit/UIProcess/ViewGestureController.h:53: _UINavigationInteractiveTransitionBase is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebKit/UIProcess/ViewGestureController.h:54: _UIViewControllerOneToOneTransitionContext is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebKit/UIProcess/ViewGestureController.h:55: _UIViewControllerTransitionContext is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebKit/UIProcess/ViewGestureController.h:132: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit/UIProcess/ViewGestureController.h:350: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit/UIProcess/mac/ViewSnapshotStoreMac.mm:73: Wrong number of spaces before statement. (expected: 4) [whitespace/indent] [4] Total errors found: 6 in 41 files If any of these errors are false positives, please file a bug against check-webkit-style.
Alice Mikhaylenko
Comment 79
2019-02-05 06:46:43 PST
Created
attachment 361187
[details]
Patch
EWS Watchlist
Comment 80
2019-02-05 06:50:53 PST
Comment hidden (obsolete)
Attachment 361187
[details]
did not pass style-queue: WARNING: File exempt from style guide. Skipping: "Source/WebKit/UIProcess/API/gtk/WebKitSettings.h" ERROR: Source/WebKit/UIProcess/ViewGestureController.h:53: _UINavigationInteractiveTransitionBase is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebKit/UIProcess/ViewGestureController.h:54: _UIViewControllerOneToOneTransitionContext is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebKit/UIProcess/ViewGestureController.h:55: _UIViewControllerTransitionContext is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebKit/UIProcess/ViewGestureController.h:132: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit/UIProcess/ViewGestureController.h:350: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit/UIProcess/mac/ViewSnapshotStoreMac.mm:73: Wrong number of spaces before statement. (expected: 4) [whitespace/indent] [4] Total errors found: 6 in 41 files If any of these errors are false positives, please file a bug against check-webkit-style.
Alice Mikhaylenko
Comment 81
2019-02-05 06:57:13 PST
Created
attachment 361188
[details]
Patch
EWS Watchlist
Comment 82
2019-02-05 07:00:38 PST
Comment hidden (obsolete)
Attachment 361188
[details]
did not pass style-queue: WARNING: File exempt from style guide. Skipping: "Source/WebKit/UIProcess/API/gtk/WebKitSettings.h" ERROR: Source/WebKit/UIProcess/ViewGestureController.h:53: _UINavigationInteractiveTransitionBase is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebKit/UIProcess/ViewGestureController.h:54: _UIViewControllerOneToOneTransitionContext is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebKit/UIProcess/ViewGestureController.h:55: _UIViewControllerTransitionContext is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebKit/UIProcess/ViewGestureController.h:132: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit/UIProcess/ViewGestureController.h:350: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit/UIProcess/mac/ViewSnapshotStoreMac.mm:73: Wrong number of spaces before statement. (expected: 4) [whitespace/indent] [4] Total errors found: 6 in 41 files If any of these errors are false positives, please file a bug against check-webkit-style.
Alice Mikhaylenko
Comment 83
2019-02-05 07:35:02 PST
Ok, the last one seems to build on all platforms.
Alice Mikhaylenko
Comment 84
2019-02-05 07:57:06 PST
Nevermind, it seems to actually break some tests rather than them being flaky.
EWS Watchlist
Comment 85
2019-02-05 08:15:45 PST
Comment hidden (obsolete)
Comment on
attachment 361188
[details]
Patch
Attachment 361188
[details]
did not pass mac-wk2-ews (mac-wk2): Output:
https://webkit-queues.webkit.org/results/11039810
New failing tests: swipe/basic-cached-back-swipe.html swipe/pushState-programmatic-back-while-swiping-crash.html swipe/pushState-cached-back-swipe.html
EWS Watchlist
Comment 86
2019-02-05 08:15:48 PST
Comment hidden (obsolete)
Created
attachment 361190
[details]
Archive of layout-test-results from ews104 for mac-highsierra-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews104 Port: mac-highsierra-wk2 Platform: Mac OS X 10.13.6
Alice Mikhaylenko
Comment 87
2019-02-06 04:32:28 PST
Created
attachment 361287
[details]
Patch
Alice Mikhaylenko
Comment 88
2019-02-06 04:47:56 PST
Created
attachment 361288
[details]
Patch
EWS Watchlist
Comment 89
2019-02-06 04:50:19 PST
Comment hidden (obsolete)
Attachment 361288
[details]
did not pass style-queue: WARNING: File exempt from style guide. Skipping: "Source/WebKit/UIProcess/API/gtk/WebKitSettings.h" ERROR: Source/WebKit/UIProcess/ViewGestureController.h:53: _UINavigationInteractiveTransitionBase is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebKit/UIProcess/ViewGestureController.h:54: _UIViewControllerOneToOneTransitionContext is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebKit/UIProcess/ViewGestureController.h:55: _UIViewControllerTransitionContext is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebKit/UIProcess/ViewGestureController.h:132: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit/UIProcess/ViewGestureController.h:350: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit/UIProcess/mac/ViewSnapshotStoreMac.mm:73: Wrong number of spaces before statement. (expected: 4) [whitespace/indent] [4] Total errors found: 6 in 40 files If any of these errors are false positives, please file a bug against check-webkit-style.
Michael Catanzaro
Comment 90
2019-02-06 09:34:44 PST
Two days past API freeze... we might try landing this anyway. :) Tim, I assume you're fine with it unless you say otherwise, since you've reviewed it. Still needs a second WPE/GTK API review.
Michael Catanzaro
Comment 91
2019-02-06 09:42:51 PST
Comment on
attachment 361288
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=361288&action=review
> Source/WebKit/UIProcess/gtk/WebMemoryPressureHandlerGtk.cpp:37 > + const char* disableMemoryPressureMonitor = getenv("WEBKIT_DISABLE_MEMORY_PRESSURE_MONITOR");
We'll need to add it to
https://trac.webkit.org/wiki/EnvironmentVariables
once this lands. Also, if we add this envvar, we should surely use it for the WebCore-level memory pressure handler as well. I've wanted WebKit-level memory pressure handler for a long time, by the way. This is cool!
Alice Mikhaylenko
Comment 92
2019-02-06 10:30:21 PST
That variable was already used for WebKitTestRunner:
https://github.com/WebKit/webkit/search?q=WEBKIT_DISABLE_MEMORY_PRESSURE_MONITOR&unscoped_q=WEBKIT_DISABLE_MEMORY_PRESSURE_MONITOR
Carlos Garcia Campos
Comment 93
2019-02-07 01:00:01 PST
Comment on
attachment 361288
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=361288&action=review
> Source/WebKit/UIProcess/API/glib/WebKitSettings.cpp:84 > + bool allowBackForwardNavigationGestures { false };
I would use enable instead of allow for this setting.
> Source/WebKit/UIProcess/API/glib/WebKitSettings.cpp:1460 > + * Enable or disable horizontal swipe gesture for back-forward navigation.
Why do we need a setting for this? Would other kind of gestures also need a setting? Should we make this more generic then?
> Source/WebKit/UIProcess/API/gtk/WebKitWebViewBase.cpp:870 > + WebKit::ViewGestureController& controller = webkitWebViewBaseViewGestureController(webViewBase);
WebKit:: is not needed here.
> Source/WebKit/UIProcess/API/gtk/WebKitWebViewBase.cpp:1175 > + WebKitWebViewBasePrivate* priv = webViewBase->priv; > + if (!priv->viewGestureController) > + priv->viewGestureController = std::make_unique<WebKit::ViewGestureController>(*priv->pageProxy); > + return *priv->viewGestureController;
gesture controller and dnd helper are created on demand because they are only created if a gesture or dnd event is received, but we are creating the view gesture controller unconditionally in webkitWebViewBaseScrollEvent() and some other places, so I would just create it in webkitWebViewBaseCreateWebPage and simply return it here.
> Source/WebKit/UIProcess/API/gtk/WebKitWebViewBase.cpp:1670 > + WebPageProxy* proxy = webkitWebViewBase->priv->pageProxy.get();
s/proxy/page/
> Source/WebKit/UIProcess/API/gtk/WebKitWebViewBase.cpp:1689 > + RefPtr<ViewSnapshot> snapshot = ViewSnapshot::create(WTFMove(surface)); > + return snapshot;
return ViewSnapshot::create(WTFMove(surface));
> Source/WebKit/UIProcess/API/gtk/WebKitWebViewBase.cpp:1694 > + if (webkitWebViewBase->priv->viewGestureController)
We don't need to null-check here if we create the view gesture controller in webkitWebViewBaseCreateWebPage. Should we check if isSwipeGestureEnabled instead?
> Source/WebKit/UIProcess/API/gtk/WebKitWebViewBase.cpp:1700 > + if (webkitWebViewBase->priv->viewGestureController)
Ditto.
> Source/WebKit/UIProcess/API/gtk/WebKitWebViewBase.cpp:1706 > + if (webkitWebViewBase->priv->viewGestureController)
Ditto.
> Source/WebKit/UIProcess/API/gtk/WebKitWebViewBase.cpp:1712 > + if (webkitWebViewBase->priv->viewGestureController)
Ditto.
> Source/WebKit/UIProcess/API/gtk/WebKitWebViewBase.cpp:1718 > + if (webkitWebViewBase->priv->viewGestureController)
Ditto.
> Source/WebKit/UIProcess/API/gtk/WebKitWebViewBase.cpp:1724 > + if (webkitWebViewBase->priv->viewGestureController)
Ditto.
> Source/WebKit/UIProcess/ViewGestureController.h:368 > + float getProgress();
getProgress -> progress() Make it const and I think it can be implemented here since it simply returns m_pogress
> Source/WebKit/UIProcess/ViewGestureController.h:369 > + SwipeDirection getDirection() { return m_direction; }
getDirection() -> direction() and make it const
> Source/WebKit/UIProcess/ViewGestureController.h:390 > + double m_velocity;
Initialize this.
> Source/WebKit/UIProcess/ViewGestureController.h:398 > + float m_startProgress; > + float m_endProgress; > + bool m_cancelled;
And these
> Source/WebKit/UIProcess/ViewGestureController.h:406 > + cairo_pattern_t* m_currentSwipeSnapshotPattern { nullptr };
Use RefPtr<cairo_pattern_t>
> Source/WebKit/UIProcess/gtk/ViewGestureControllerGtk.cpp:115 > + GtkWidget* widget = m_viewGestureController.m_webPageProxy.viewWidget();
There's m_webPageProxy in ViewGestureController::SwipeProgressTracker
> Source/WebKit/UIProcess/gtk/ViewGestureControllerGtk.cpp:151 > + if ( > +#if GTK_CHECK_VERSION(3, 20, 0) > + event->is_stop > +#else > + !event->delta_x && !event->delta_y > +#endif > + ) {
This is difficult to read, add a helper function isEventStop() or something like that and move the #if there. It can be used in scrollEventCanEndSwipe too.
> Source/WebKit/UIProcess/gtk/ViewGestureControllerGtk.cpp:165 > + m_progress = std::min(std::max(m_progress, minProgress), maxProgress);
clampTo ?
> Source/WebKit/UIProcess/gtk/ViewGestureControllerGtk.cpp:194 > + GtkWidget* widget = m_viewGestureController.m_webPageProxy.viewWidget();
m_webPageProxy
> Source/WebKit/UIProcess/gtk/ViewGestureControllerGtk.cpp:247 > + ViewSnapshot* snapshot = targetItem->snapshot(); > + if (snapshot) {
if (auto* snapshot = targetItem->snapshot()) {
> Source/WebKit/UIProcess/gtk/ViewGestureControllerGtk.cpp:250 > + FloatSize viewSize = FloatSize(m_webPageProxy.viewSize());
FloatSize viewSize(m_webPageProxy.viewSize());
> Source/WebKit/UIProcess/gtk/ViewGestureControllerGtk.cpp:258 > + m_currentSwipeSnapshotPattern = cairo_pattern_create_rgba(color.red(), color.green(), color.blue(), color.alpha());
Does this work? Cairo expects doubles between 0 and 1, but WebCore::Color uses integers between 0 and 255. You should use color.getRGBA passing doubles
> Source/WebKit/UIProcess/gtk/ViewSnapshotStoreGtk.cpp:70 > + return stride*height;
stride * height
> Source/WebKit/UIProcess/gtk/ViewSnapshotStoreGtk.cpp:76 > + return WebCore::IntSize();
return { };
> Source/WebKit/UIProcess/gtk/ViewSnapshotStoreGtk.cpp:82 > + return WebCore::IntSize(width, height);
return { width, height };
> Source/WebKit/UIProcess/gtk/WebMemoryPressureHandlerGtk.cpp:39 > + if (disableMemoryPressureMonitor && !strcmp(disableMemoryPressureMonitor, "1")) > + return;
I think it should be the caller the one checking the env var before calling this.
> Source/WebKit/UIProcess/gtk/WebMemoryPressureHandlerGtk.cpp:48 > + auto& memoryPressureHandler = MemoryPressureHandler::singleton(); > + memoryPressureHandler.setLowMemoryHandler([] (Critical critical, Synchronous) { > + ViewSnapshotStore::singleton().discardSnapshotImages(); > + > + for (auto* processPool : WebProcessPool::allProcessPools()) > + processPool->handleMemoryPressureWarning(critical); > + }); > + memoryPressureHandler.install();
Maybe we could share all this with mac port too. If we ever need to add platform specific stuff we can add a platform method inside the lambda. We would also need to move the
> Source/WebKit/UIProcess/gtk/WebProcessPoolGtk.cpp:80 > + > + installMemoryPressureHandler();
We could check the env var here once, storing its result in a static and then use it also in platformInitializeWebProcess below.
Alice Mikhaylenko
Comment 94
2019-02-07 04:12:39 PST
> Why do we need a setting for this? Would other kind of gestures also need a setting? Should we make this more generic then?
It's only useful for browsers, but not e.g. for auth widgets. So I assume very few apps will actually want this, like Epiphany, Yelp, Devhelp in GNOME, but not auth widgets in control center, or RH login in Boxes, or Google Docs widget in Documents, or mail display in Geary, etc.
> Does this work? Cairo expects doubles between 0 and 1, but WebCore::Color uses integers between 0 and 255. You should use color.getRGBA passing doubles
Ahh. I just assumed every page I tested had white background. :(
> Maybe we could share all this with mac port too. If we ever need to add platform specific stuff we can add a platform method inside the lambda. We would also need to move the
I wanted to, but the Cocoa one has this comment: // FIXME: This should be able to share code with WebCore's MemoryPressureHandler (and be platform independent). // Right now it cannot because WebKit1 and WebKit2 need to be able to coexist in the UI process, // and you can only have one WebCore::MemoryPressureHandler. We can though add it directly there specifically with #if PLATFORM(GTK) and drop the added memory pressure handler. The rest: sure
Carlos Garcia Campos
Comment 95
2019-02-07 22:23:50 PST
(In reply to Alexander Mikhaylenko from
comment #94
)
> > Why do we need a setting for this? Would other kind of gestures also need a setting? Should we make this more generic then? > > It's only useful for browsers, but not e.g. for auth widgets. So I assume > very few apps will actually want this, like Epiphany, Yelp, Devhelp in > GNOME, but not auth widgets in control center, or RH login in Boxes, or > Google Docs widget in Documents, or mail display in Geary, etc.
Would other kind of gestures also need a setting? Should we make this more generic then? I mean future gestures.
> > Does this work? Cairo expects doubles between 0 and 1, but WebCore::Color uses integers between 0 and 255. You should use color.getRGBA passing doubles > > Ahh. I just assumed every page I tested had white background. :( > > > Maybe we could share all this with mac port too. If we ever need to add platform specific stuff we can add a platform method inside the lambda. We would also need to move the > > I wanted to, but the Cocoa one has this comment: > > // FIXME: This should be able to share code with WebCore's > MemoryPressureHandler (and be platform independent). > // Right now it cannot because WebKit1 and WebKit2 need to be able to > coexist in the UI process, > // and you can only have one WebCore::MemoryPressureHandler. > > We can though add it directly there specifically with #if PLATFORM(GTK) and > drop the added memory pressure handler.
We can make this cross-platform in webkit anyway, just move the check to the caller as I'm suggesting for the GTK case.
> The rest: sure
Alice Mikhaylenko
Comment 96
2019-02-08 00:02:19 PST
> Would other kind of gestures also need a setting? Should we make this more generic then? I mean future gestures.
Tbh I don't think there will be any more WebKit-side gestures. There are potential gestures for browsers in general, like swipe to switch tabs, or pull to refresh, but that doesn't involve WebKit. There can be a variant of the same gesture for touchscreens though (say, an edge swipe), but it can use the same setting. As for the existing ones, the touchscreen ones (pan, long press etc) are completely necessary, so I don't think it makes sense to be able to disable them. And pinch zoom (on both devices) is more debatable, but why would an app disable it?
Tim Horton
Comment 97
2019-02-08 00:12:35 PST
(In reply to Alexander Mikhaylenko from
comment #96
)
> > Would other kind of gestures also need a setting? Should we make this more generic then? I mean future gestures. > > Tbh I don't think there will be any more WebKit-side gestures. There are > potential gestures for browsers in general, like swipe to switch tabs, or > pull to refresh, but that doesn't involve WebKit. > > There can be a variant of the same gesture for touchscreens though (say, an > edge swipe), but it can use the same setting. > > As for the existing ones, the touchscreen ones (pan, long press etc) are > completely necessary, so I don't think it makes sense to be able to disable > them. And pinch zoom (on both devices) is more debatable, but why would an > app disable it?
FWIW a lot of WebKit embeds on macOS don't want the pinch gesture. We have a separate bit for each (swipe and pinch).
Alice Mikhaylenko
Comment 98
2019-02-08 01:12:52 PST
Hm.
https://developer.apple.com/documentation/webkit/wkwebview/1414983-allowsmagnification?language=objc
this one? There already is a pinch gesture that is non-optional in WebKitGTK+ though. And the touchscreen one has been available for quite some time now, though there hasn't been a release with the touchpad one yet.
Alice Mikhaylenko
Comment 99
2019-02-08 09:59:21 PST
Created
attachment 361512
[details]
Patch
EWS Watchlist
Comment 100
2019-02-08 10:02:30 PST
Comment hidden (obsolete)
Attachment 361512
[details]
did not pass style-queue: WARNING: File exempt from style guide. Skipping: "Source/WebKit/UIProcess/API/gtk/WebKitSettings.h" ERROR: Source/WebKit/UIProcess/ViewGestureController.h:53: _UINavigationInteractiveTransitionBase is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebKit/UIProcess/ViewGestureController.h:54: _UIViewControllerOneToOneTransitionContext is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebKit/UIProcess/ViewGestureController.h:55: _UIViewControllerTransitionContext is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebKit/UIProcess/ViewGestureController.h:133: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit/UIProcess/ViewGestureController.h:351: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit/UIProcess/mac/ViewSnapshotStoreMac.mm:73: Wrong number of spaces before statement. (expected: 4) [whitespace/indent] [4] Total errors found: 6 in 41 files If any of these errors are false positives, please file a bug against check-webkit-style.
Alice Mikhaylenko
Comment 101
2019-02-08 10:05:08 PST
Addressed
comment 93
. I'm not completely sure that's what you meant to do with WebMemoryPressureHandler, so maybe still missing something. Also, added a gtk_widget_queue_draw() call to PageClientImpl::didRemoveNavigationGestureSnapshot(), so that we redraw the widget immediately after removing snapshot (otherwise, there's a rare bug where snapshot is still displayed until you scroll/cause redraws otherwise)
Michael Catanzaro
Comment 102
2019-02-08 10:49:21 PST
EWS is red
Alice Mikhaylenko
Comment 103
2019-02-08 11:44:44 PST
Created
attachment 361519
[details]
Patch
Alice Mikhaylenko
Comment 104
2019-02-08 11:45:20 PST
(In reply to Michael Catanzaro from
comment #102
)
> EWS is red
Oh, right, there was one more thing to do in project.pbxproj
EWS Watchlist
Comment 105
2019-02-08 11:46:36 PST
Comment hidden (obsolete)
Attachment 361519
[details]
did not pass style-queue: WARNING: File exempt from style guide. Skipping: "Source/WebKit/UIProcess/API/gtk/WebKitSettings.h" ERROR: Source/WebKit/UIProcess/ViewGestureController.h:53: _UINavigationInteractiveTransitionBase is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebKit/UIProcess/ViewGestureController.h:54: _UIViewControllerOneToOneTransitionContext is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebKit/UIProcess/ViewGestureController.h:55: _UIViewControllerTransitionContext is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebKit/UIProcess/ViewGestureController.h:133: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit/UIProcess/ViewGestureController.h:351: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit/UIProcess/mac/ViewSnapshotStoreMac.mm:73: Wrong number of spaces before statement. (expected: 4) [whitespace/indent] [4] Total errors found: 6 in 41 files If any of these errors are false positives, please file a bug against check-webkit-style.
Alice Mikhaylenko
Comment 106
2019-02-08 12:16:56 PST
Created
attachment 361522
[details]
Patch
EWS Watchlist
Comment 107
2019-02-08 12:20:18 PST
Comment hidden (obsolete)
Attachment 361522
[details]
did not pass style-queue: WARNING: File exempt from style guide. Skipping: "Source/WebKit/UIProcess/API/gtk/WebKitSettings.h" ERROR: Source/WebKit/UIProcess/ViewGestureController.h:53: _UINavigationInteractiveTransitionBase is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebKit/UIProcess/ViewGestureController.h:54: _UIViewControllerOneToOneTransitionContext is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebKit/UIProcess/ViewGestureController.h:55: _UIViewControllerTransitionContext is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebKit/UIProcess/ViewGestureController.h:133: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit/UIProcess/ViewGestureController.h:351: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit/UIProcess/mac/ViewSnapshotStoreMac.mm:73: Wrong number of spaces before statement. (expected: 4) [whitespace/indent] [4] Total errors found: 6 in 42 files If any of these errors are false positives, please file a bug against check-webkit-style.
Alice Mikhaylenko
Comment 108
2019-02-08 12:47:08 PST
Created
attachment 361524
[details]
Patch
EWS Watchlist
Comment 109
2019-02-08 12:50:46 PST
Comment hidden (obsolete)
Attachment 361524
[details]
did not pass style-queue: WARNING: File exempt from style guide. Skipping: "Source/WebKit/UIProcess/API/gtk/WebKitSettings.h" ERROR: Source/WebKit/UIProcess/ViewGestureController.h:53: _UINavigationInteractiveTransitionBase is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebKit/UIProcess/ViewGestureController.h:54: _UIViewControllerOneToOneTransitionContext is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebKit/UIProcess/ViewGestureController.h:55: _UIViewControllerTransitionContext is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebKit/UIProcess/ViewGestureController.h:133: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit/UIProcess/ViewGestureController.h:351: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit/UIProcess/mac/ViewSnapshotStoreMac.mm:73: Wrong number of spaces before statement. (expected: 4) [whitespace/indent] [4] Total errors found: 6 in 42 files If any of these errors are false positives, please file a bug against check-webkit-style.
Alice Mikhaylenko
Comment 110
2019-02-08 13:00:13 PST
Created
attachment 361527
[details]
Patch
EWS Watchlist
Comment 111
2019-02-08 13:03:27 PST
Comment hidden (obsolete)
Attachment 361527
[details]
did not pass style-queue: WARNING: File exempt from style guide. Skipping: "Source/WebKit/UIProcess/API/gtk/WebKitSettings.h" ERROR: Source/WebKit/UIProcess/ViewGestureController.h:53: _UINavigationInteractiveTransitionBase is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebKit/UIProcess/ViewGestureController.h:54: _UIViewControllerOneToOneTransitionContext is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebKit/UIProcess/ViewGestureController.h:55: _UIViewControllerTransitionContext is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebKit/UIProcess/ViewGestureController.h:133: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit/UIProcess/ViewGestureController.h:351: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit/UIProcess/mac/ViewSnapshotStoreMac.mm:73: Wrong number of spaces before statement. (expected: 4) [whitespace/indent] [4] Total errors found: 6 in 42 files If any of these errors are false positives, please file a bug against check-webkit-style.
Alice Mikhaylenko
Comment 112
2019-02-08 13:13:28 PST
Created
attachment 361528
[details]
Patch
EWS Watchlist
Comment 113
2019-02-08 13:16:23 PST
Comment hidden (obsolete)
Attachment 361528
[details]
did not pass style-queue: WARNING: File exempt from style guide. Skipping: "Source/WebKit/UIProcess/API/gtk/WebKitSettings.h" ERROR: Source/WebKit/UIProcess/ViewGestureController.h:53: _UINavigationInteractiveTransitionBase is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebKit/UIProcess/ViewGestureController.h:54: _UIViewControllerOneToOneTransitionContext is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebKit/UIProcess/ViewGestureController.h:55: _UIViewControllerTransitionContext is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebKit/UIProcess/ViewGestureController.h:133: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit/UIProcess/ViewGestureController.h:351: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit/UIProcess/mac/ViewSnapshotStoreMac.mm:73: Wrong number of spaces before statement. (expected: 4) [whitespace/indent] [4] Total errors found: 6 in 41 files If any of these errors are false positives, please file a bug against check-webkit-style.
Alice Mikhaylenko
Comment 114
2019-02-08 13:43:28 PST
Ok, it builds now. Also, the patch doesn't add or remove any @no-unify for Cocoa anymore, so that's solved as well.
Michael Catanzaro
Comment 115
2019-02-08 17:01:34 PST
Comment on
attachment 361528
[details]
Patch Let's try it.
WebKit Commit Bot
Comment 116
2019-02-08 17:29:29 PST
Comment on
attachment 361528
[details]
Patch Clearing flags on attachment: 361528 Committed
r241224
: <
https://trac.webkit.org/changeset/241224
>
WebKit Commit Bot
Comment 117
2019-02-08 17:29:33 PST
All reviewed patches have been landed. Closing bug.
Carlos Garcia Campos
Comment 118
2019-02-09 00:42:47 PST
(In reply to Alexander Mikhaylenko from
comment #101
)
> Addressed
comment 93
. I'm not completely sure that's what you meant to do > with WebMemoryPressureHandler, so maybe still missing something.
Exactly what I meant, thanks!
> Also, added a gtk_widget_queue_draw() call to > PageClientImpl::didRemoveNavigationGestureSnapshot(), so that we redraw the > widget immediately after removing snapshot (otherwise, there's a rare bug > where snapshot is still displayed until you scroll/cause redraws otherwise)
Alice Mikhaylenko
Comment 119
2019-02-09 02:37:41 PST
And a followup:
https://bugs.webkit.org/show_bug.cgi?id=194472
:(
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug