Introduce ScrollCustomization Property [chromium/src : master]
Ehsan Karamad (Gerrit)
Hello Majid, PTAL.
To view, visit change 590497. To unsubscribe, or for help writing mail filters, visit settings.
Majid Valipour (Gerrit)
13 comments:
File cc/input/scroll_customization.h:
I think the new preferred way to do these type of bitflags is to use struct with constant integer values. See for example MutableProperties [1].
In particular, use uint32_t to represent the "scroll_customization_" value which is a bitset
of the above flags.I am not sure if this is correct for example if one passes in
kScrollCustomizationPanLeft and kScrollCustomizationPanUp. the
result is not a valid value in ScrollCustomization.See above comment on how to define a safe bitflag.
Patch Set #9, Line 53:
touch_actions/touch_action/scroll_customization/
Patch Set #9, Line 94:
GetScrollCustomizationFromScrollStateDatahmmm, you are actually returning a "mask" so perhaps
|GetScrollCustomizationMaskFromScrollStateData|why is this inline?
Patch Set #9, Line 98:
delta_xI have seen this being used a few times, should we have a helper method on
ScrollStateDate e.g., GetEffectiveDelta{X, Y}() which returns this
value?That way you don't need to pass in the whole scrollStateData but just the
effective delta.
nit: layout tests should not have <html> <head> <body> [1]
I think it will be nicer to have this method take a gesture value instead of delta and
then turn that into specific delta values.Also it will be cleaner for it to return a promise.
Then usage will be like:
applyScrollGesture('right').then(verify);Patch Set #9, Line 82:
var applyScrollDidChange = callsToApplyScroll < numberOfSetApplyScrollCalls;This is hard to read and understand. I think what you want is to set the
|numberOfSetApplyScrollCalls| to zero just above and check here it it is
none zero. Also get rid of callsToApplyScroll etc.Patch Set #9, Line 107:
numberOfSetApplyScrollCallsSee my comment above. These steps should be done for each test.
Perhaps it should be part of a "SetUp" step for each test.Patch Set #9, Line 110:
if ('ScrollState' in window) {We should turn this conditional into an early return at the top of the script.
Patch Set #9, Line 121:
testGestureMost tests below do the same thing. Perhaps these should be a parametric test.
Each table row will have an entry like this:const test_table = [
[GESTURE_RIGHT, GESTURE_LEFT, GESTURE_TOP, GESTURE_DOWN], "none", , false]
....
];
So
test_table.forEach([gestures, value, expected, test_prefix]) {
gestures.forEach(gesture) {
testGesture(value, gesture, expected, `Verify if property "${value}" works correctly with gesture ${gesture}`);
}
}The test() is actually invoked inside testGesture. This way you get an individual test for
each combination and also get rid of most of the boilerplate.bonus 1: make GESTURE_RIGHT etc strings so you can print them properly.
bonus 2: each test is verifying a set gestures for true and the reverse of that set for false. I think you can event remove this bilerplate and generate both set of tests from a single entry in
test_table.
File third_party/WebKit/Source/core/dom/Element.cpp:
Patch Set #9, Line 637:
NativeDistributeScrollThis should be merged with previous conditional e.g.,
bool may_customize_scroll_for_gesture = direction & scroll_customization
if (!callback || disable_custom_callbacks || !may_customize_scroll_for_gesture)
[...]Patch Set #9, Line 731:
// Cache the current value of ScrollCustomization to later verify the handlerIs this comment accurate? No value is being cached atm.
To view, visit change 590497. To unsubscribe, or for help writing mail filters, visit settings.
Majid Valipour (Gerrit)
1 comment:
I think the new preferred way to do these type of bitflags is to use struct with constant integer va […]
Actually looking at it more carefully. I think you may need to use the current
pattern as it is how StyleBuilderConverter::ConvertFlags expects it to behave.So I am fine with this given that it matches TouchAction as well.
To view, visit change 590497. To unsubscribe, or for help writing mail filters, visit settings.
Ehsan Karamad (Gerrit)
Thanks Majid! PTAL.
13 comments:
I think the new preferred way to do these type of bitflags is to use struct with constant integer va […]
Thanks. I was about to suggest the same. I could use "uint32_t" for enum though but I am not sure if it is needed/suggested (e.g., "enum ScrollCustomization : uint32 {").
I am not sure if this is correct for example if one passes in […]
I think this falls into the same question above and AFAICT this is correct. It is vague as in for the example you mentioned we do not "define" a value. However, the corresponding "int" value is within the accepted range for ScrollCustomization but the value is not equal to any of the names mentioned. Nonetheless, we do bit manipulation on this so it should be fine (also ditto
on being derived from TouchAction logic).
Patch Set #9, Line 53:
touch_actions/touch_action/scroll_customization/
Thanks. Done.
Patch Set #9, Line 94:
GetScrollCustomizationFromScrollStateDatahmmm, you are actually returning a "mask" so perhaps
|GetScrollCustomizationMaskFromScrollStateData|
I don't see this as a "mask" TBH. This is actually the direction of scrolling based on X & Y. I personally consider the output as "direction" rather than mask, esp. given that we do look at X and Y (also we do return a ScrollCustomization type so GetScrollCustomization is sounds like the safest name to me).
That being said I am quite open to renaming the method if you believe Mask is a better term.
why is this inline?
Thanks for pointing this out. I am open to any suggestions for this. Here is the situation:Adding the inline modifier will make sure that we get different definitions of inline in different translation units. Without inline there will be link errors due to multiple definitions of the cc::GetScrollCustomizationFromScrollStateData.
We could remove inline and add the implementation to a cc file but then the cc file will be inside cc/input and we cannot rely on it from inside blink.
We could mark it as static inline and leave the implementation in cc-file but then that will be external linkage which I am not quite sure if it is considered as a good approach in coding (+ we are again using "inline" anyway).
Anything I am missing/suggests?
I have seen this being used a few times, should we have a helper method on […]
Thanks. Good suggestion! I believe you are referring to examples such as this: https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/input/ScrollManager.cpp?rcl=2cf2b39b0730d15af71ac6e1e24125bcce663fe0&l=142
I think it is a good idea but it should perhaps be added to ScrollState as opposed to ScrollStateData which is more of a "Raw" value.
I have added ScrollState::effectiveDeltaX/Y(). If this sticks I can go ahead (later) and refactor the code base to use the newly added methods).
nit: layout tests should not have <html> <head> <body> [1] […]
Ack
I think it will be nicer to have this method take a gesture value instead of delta and […]
Done. Thanks for the suggestion.
Patch Set #9, Line 82:
deltaY = deltaThis is hard to read and understand. I think what you want is to set the […]
Hopefully the new one makes more sense. (I believe) I now test all (16) possible configurations of "scroll-customization" against all 4 gestures.
Patch Set #9, Line 110:
powerSet.add(s);
We should turn this conditional into an early return at the top of the script.
I am wondering if it is even needed. Wouldn't the assert on line 39 exit the layout test altogether?
Patch Set #9, Line 121:
stureSet.siMost tests below do the same thing. Perhaps these should be a parametric test. […]
Thanks for the suggestion. I think I did something similar in the newer version. Note quite sure if I understand bonus #2 though.
Patch Set #12, Line 165:
}, `Verify if property "${scrollCustomization}" works correctly with gesture "${testGesture}" given that the set of valid gestures is: {${validGestureSetString}}".`);Looking at this again, the log message might be too long. I could maybe drop the last sentence.
File third_party/WebKit/Source/core/dom/Element.cpp:
Patch Set #9, Line 637:
callback->handleEvent(This should be merged with previous conditional e.g., […]
Sure. Done. The reason I did not merge this in the first place was to get a bit of efficiency given that perhaps most of the time an element does not have a callback so we should not try to get the scroll customization (which is kNone) and try to get direction.
Is this comment accurate? No value is being cached atm.
Thanks! Fixed. I also think we do not need to cache any value anymore (following our discussion offline).
To view, visit change 590497. To unsubscribe, or for help writing mail filters, visit settings.
Majid Valipour (Gerrit)
8 comments:
File cc/input/scroll_customization.h:
Patch Set #12, Line 16:
uint32_t32 bit is overkill. 8 buts should be more that enough.
Patch Set #12, Line 97:
epsilonPlease use the same kEpsilon value as we use here [1] to match existing scrolling code.
Also make sure to leave a TODO to the same issue.
File cc/input/scroll_customization.h:
Thanks. I was about to suggest the same. […]
It is fine to follow the TouchAction pattern here.
I think this falls into the same question above and AFAICT this is correct. […]
Ack
Patch Set #9, Line 94:
GetScrollCustomizationFromScrollStateDataIts type is ScrollCustomization but it is used as a mask to filter only applicable
values from element scroll customization value. I am fine with either using ScrollCustomizationDirection or ScrollCustomizationMask but I think
the name change helps to avoid confusion with Element.GetScrollCustomization.
Adding the inline modifier will make sure that we get different definitions of inline in different translation units. Without inline there will be link errors due to multiple definitions of the cc::GetScrollCustomizationFromScrollStateData.
I don't think you get a link error due to duplicate definition but rather you may be
getting one because the cc definition is not exported. Try exporting it and see if
inline is no longer needed.
Thanks. Good suggestion! I believe you are referring to examples such as this: https://cs.chromium. […]
sgtm.
This function does not seem to be testing pan-left, pan-right ...
As a meta comment, I think the test much better now but the new patch is
taking the test parametrization too far and making the test too complex.Now to actually understand the test one has to fully understand getScrollCustomizationFromValidGestureSet
and how this machinery relies on getPowerSet, validGestureSet and ALL_GESTURE
too generate all possible inputs. This makes the test less readable and any
bug in input generation can lead to test not being effective. In general making
the test too complex is not a good idea.I think the test should be parametrized but please use constant static input
values as opposed to trying to generate them. In this case the number of
permutations is not that many so there is not actually much that the input generation
is saving.A simple table of ||scroll-customization value | gestures to be tests| callbacks expected||
should work. e.g.const test_table = [
["none", [GESTURE_RIGHT, GESTURE_LEFT, GESTURE_TOP, GESTURE_DOWN] , false]
["pan-left", [GESTURE_RIGHT, GESTURE_TOP, GESTURE_DOWN] , false]
["pan-left", [GESTURE_LEFT] , , true]
["pan-x", [GESTURE_TOP, GESTURE_DOWN] , false]
["pan-x", [GESTURE_LEFT, GESTURE_RIGHT] , , true]
....
];
> Note quite sure if I understand bonus #2 though.
It was suggesting that for example in above table item 3 can be automatically generated
based on item 2. But I think not doing that is actually better for readability of
the test in light of above discussion.nit: can be forEach(testGestureSet)
To view, visit change 590497. To unsubscribe, or for help writing mail filters, visit settings.
Ehsan Karamad (Gerrit)
Thanks Maijd! PTAL.
9 comments:
32 bit is overkill. 8 buts should be more that enough.
Done
Please use the same kEpsilon value as we use here [1] to match existing scrolling code. […]
Done
File cc/input/scroll_customization.h:
It is fine to follow the TouchAction pattern here.
Done
Ack
Done
Patch Set #9, Line 53:
touch_action
Thanks. Done.
Done
Its type is ScrollCustomization but it is used as a mask to filter only applicable
values from element scroll customization value. I am fine with either using ScrollCustomizationDirection or ScrollCustomizationMask but I think
the name change helps to avoid confusion with Element.GetScrollCustomization.
I think something like GetScrollCustomizationFromDirection() might make more sense. This means a corresponding ScrollCustomization setting which allows the direction. FWIW this is gotten from input and (I think) the actual mask here would be Element.GetScrollCustomization().
> Adding the inline modifier will make sure that we get different definitions of inline in different translation units. Without inline there will be link errors due to multiple definitions of the cc::GetScrollCustomizationFromScrollStateData.
> I don't think you get a link error due to duplicate definition but rather you may be
getting one because the cc definition is not exported. Try exporting it and see if
inline is no longer needed.
Thanks. You actually do get a link error due to multiple definition which has to do with the many places the include (V8* files) ScrollCustomization.h Naturally, "in-lining" fixes the issue but not the idea fix.
The alternate way I tried and failed was to move the implementation to a separate CC file. However the failure reason for that was the lack of CC_EXPORT.
sgtm.
Done
Patch Set #12, Line 142:
" and distributeScroll handler methods.");
This function does not seem to be testing pan-left, pan-right ...
Wouldn't "scroll-customization: pan-left pan-right" just mean "scroll-customization: pan-x". That being said, if either one of "pan-left" and "pan-right" is in |gestureSet| it will be in |summarized| per line 124. I still don't understand why it would not work (I believe it does work).
As a meta comment, I think the test much better now but the new patch is
taking the test parametrization too far and making the test too complex.
The reason I opted for "input generation" was that we are testing all possible compositions of valid gestures against all possible gestures which is of size 16 x 4 = 64.
Now to actually understand the test one has to fully understand getScrollCustomizationFromValidGestureSet
and how this machinery relies on getPowerSet, validGestureSet and ALL_GESTURE
too generate all possible inputs. This makes the test less readable and any
bug in input generation can lead to test not being effective. In general making
the test too complex is not a good idea.
I agree the test should be simple. A few notes:
* getPowerSet approach is simply listing all possible configurations which is more efficient than listing the 64 cases. When the input is all input I do not see a lot of sense if listing them explicitly. Perhaps renaming the method to "getAllScrollCustomizationSettings" would alleviate the problem more.* getScrollCustomizationFromValidGestureSet: This is the helper class which I agree might make the inspection of the test harder.
So in conclusion and based on out offline chat I will try to use a table like the one you suggested.
> I think the test should be parametrized but please use constant static input
> values as opposed to trying to generate them.
I consider the 4 gestures and constant static. If we are to test a subset of current tests I totally understand and agree with you that defining constants and tables is a more readable approach and what I have here is an overkill. If we want to test all cases I do not find writing a table that appealing. All in all, I defer the decision to you given that you are wat more experienced with blink/ and layout testing.
> In this case the number of
> permutations is not that many so there is not actually much that the input generation
> is saving.
>
> A simple table of ||scroll-customization value | gestures to be tests| callbacks expected||
> should work. e.g.
>
> const test_table = [
> ["none", [GESTURE_RIGHT, GESTURE_LEFT, GESTURE_TOP, GESTURE_DOWN] , false]
> ["pan-left", [GESTURE_RIGHT, GESTURE_TOP, GESTURE_DOWN] , false]
> ["pan-left", [GESTURE_LEFT] , , true]
> ["pan-x", [GESTURE_TOP, GESTURE_DOWN] , false]
> ["pan-x", [GESTURE_LEFT, GESTURE_RIGHT] , , true]
> ....
> ];
I will add a similar table.
nit: can be forEach(testGestureSet)
Thanks.
To view, visit change 590497. To unsubscribe, or for help writing mail filters, visit settings.
Majid Valipour (Gerrit)
lgtm with nits.
Please update the description to explain how the scroll customization is used i.e., to limit calling scroll customization callbacks only base on this declared
values thus enhancing performance for composited scrolling.
Also description talks about sending these values to WebLayer which is not the
case as of yet. I think that is to happen in follow up CL which wires this up
to CC.
9 comments:
Patch Set #13, Line 54:
result?s/result?/scroll customization handler was called/
Patch Set #13, Line 137:
resetStatenit: Is this still needed? The state is reset at the begging of each test case.
Patch Set #13, Line 139:
scrollCustomizationHandlersCallednit: s/scrollCustomizationHandlersCalled/didCallScrollCustomizationHandlers/g
File third_party/WebKit/Source/core/dom/Element.cpp:
Patch Set #13, Line 620:
ScrollCustomization scroll_customization =nit: It think it will be nice if you move this into and small utility function in source file
and also use it below.bool HasScrollCustomizationForGesture(const Layoutbox&, const ScrollState&)
nit: unnecessary blank line.
File third_party/WebKit/Source/core/page/scrolling/ScrollState.h:
Patch Set #13, Line 71:
and deltaX() otherwisenit: s/and deltaX() otherwise/otherwise deltaX()/
Without this change it reads a bit odd to me.
Patch Set #13, Line 73:
isBeginningnit: should we use data_->is_beginning here? the function call seems unnecessary and
inefficient. Same for daltaX and deltaXHint() etc.Patch Set #13, Line 76:
and deltaY() otherwisenit: ditto.
Patch Set #13, Line 78:
isBeginningnit: ditto
To view, visit change 590497. To unsubscribe, or for help writing mail filters, visit settings.
Ehsan Karamad (Gerrit)
Ehsan Karamad uploaded patch set #15 to this change.
Introduce ScrollCustomization Property
This CL introduces the CSS property, 'scroll-customization'. Through modifying
scroll customization unnecessary calls to apply/distributeScroll handles
can be avoided.
This CL implements the logic inside blinks main thread. If an scrollable node
has customized scrolling handlers, it can further specify which class
of gestures it is interested for custom scrolling.
The list of valid values for customization includes 'auto', 'pan-x',
'pan-left', 'pan-right', 'pan-y', 'pan-up', and 'pan-down'.
This CL will eventually be followed up by changes to the compositor layer
so that for nodes with custom scroll, the scroll events corresponding
not allowed through scroll customization do not get sent to the main thread.
Bug:
Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel
Change-Id: Ia54971b5535cb0b841682d242ef90761975f8c81
---
M cc/BUILD.gn
A cc/input/scroll_customization.cc
A cc/input/scroll_customization.h
A third_party/WebKit/LayoutTests/virtual/scroll_customization/fast/scroll-behavior/scroll-customization-property.html
M third_party/WebKit/Source/core/BUILD.gn
M third_party/WebKit/Source/core/css/BUILD.gn
M third_party/WebKit/Source/core/css/CSSPrimitiveValueMappings.h
M third_party/WebKit/Source/core/css/CSSProperties.json5
M third_party/WebKit/Source/core/css/CSSValueKeywords.json5
M third_party/WebKit/Source/core/css/ComputedStyleCSSValueMapping.cpp
A third_party/WebKit/Source/core/css/properties/CSSPropertyAPIScrollCustomization.cpp
M third_party/WebKit/Source/core/dom/Element.cpp
M third_party/WebKit/Source/core/frame/UseCounter.cpp
M third_party/WebKit/Source/core/page/scrolling/ScrollState.h
M third_party/WebKit/Source/devtools/front_end/sdk/CSSMetadata.js
M third_party/WebKit/Source/platform/BUILD.gn
A third_party/WebKit/Source/platform/scroll/ScrollCustomization.cpp
A third_party/WebKit/Source/platform/scroll/ScrollCustomization.h
M tools/metrics/histograms/enums.xml
19 files changed, 553 insertions(+), 5 deletions(-)
To view, visit change 590497. To unsubscribe, or for help writing mail filters, visit settings.
Ehsan Karamad (Gerrit)
Patch Set 13:
(9 comments)
lgtm with nits.
Please update the description to explain how the scroll customization is used i.e., to limit calling scroll customization callbacks only base on this declared
values thus enhancing performance for composited scrolling.Also description talks about sending these values to WebLayer which is not the
case as of yet. I think that is to happen in follow up CL which wires this up
to CC.
Yes thanks the description was fairly outdated and insufficient. PTAL. Also, do we have an umbrella bug I should add to the CL?
9 comments:
s/result?/scroll customization handler was called/
Done
Patch Set #13, Line 137:
element.
nit: Is this still needed? The state is reset at the begging of each test case.
I added this line to instantiate/define variables used in lines 133-134 and 140-143. But it is not really needed. While still not needed, I added explicit definition of the variables to where I am defining constants.
nit: s/scrollCustomizationHandlersCalled/didCallScrollCustomizationHandlers/g
Done
File third_party/WebKit/Source/core/dom/Element.cpp:
nit: It think it will be nice if you move this into and small utility function in source file […]
I refactored them both in this file. Maybe it looks nicer if it were in ScrollCustomization.cpp but there seems to be layering violations (cannot include core/ from platform/).
Patch Set #13, Line 728:
!has_scroll_customization_for_gesture) {nit: unnecessary blank line.
Hmm...while use of empty spaces is discouraged I believe this should help with readability given that the decision on lines 723-727 is about "whether or not we call the handlers" and the lines 729-737 is about "the order of calling handlers". But this is subjective so I revert back to how it looked before.
https://google.github.io/styleguide/cppguide.html#Vertical_Whitespaceditto for line 631.
File third_party/WebKit/Source/core/page/scrolling/ScrollState.h:
nit: s/and deltaX() otherwise/otherwise deltaX()/ […]
Ack
nit: should we use data_->is_beginning here? the function call seems unnecessary and […]
Done. I simply used the methods to make it cleaner and performance wise it should not change as perhaps all these methods will be inline. That being said, taking a second look, it would look more consistent if we use |data_| like in other methods.
Patch Set #13, Line 76:
hint : data_->delta_y;nit: ditto.
Ack
nit: ditto
Ack
To view, visit change 590497. To unsubscribe, or for help writing mail filters, visit settings.
Majid Valipour (Gerrit)
Majid Valipour (Gerrit)
Majid Valipour would like Timothy Dresser to review this change.
19 files changed, 558 insertions(+), 7 deletions(-)
Ehsan Karamad (Gerrit)
Ehsan Karamad uploaded patch set #18 to this change.
Introduce ScrollCustomization Property
This CL introduces the CSS property, 'scroll-customization'. Through modifying
scroll customization unnecessary calls to apply/distributeScroll handles
can be avoided.
This CL implements the logic inside blinks main thread. If an scrollable node
has customized scrolling handlers, it can further specify which class
of gestures it is interested for custom scrolling.
The list of valid values for customization includes 'auto', 'pan-x',
'pan-left', 'pan-right', 'pan-y', 'pan-up', and 'pan-down'.
This CL will eventually be followed up by changes to the compositor layer
so that for nodes with custom scroll, the scroll events corresponding
not allowed through scroll customization do not get sent to the main thread.
Bug: 765326
Majid Valipour (Gerrit)
1 comment:
File cc/input/scroll_customization.h:
nit: please remove cmath and string includes. There are not used.
To view, visit change 590497. To unsubscribe, or for help writing mail filters, visit settings.
Timothy Dresser (Gerrit)
Can we add some unit tests for this?
4 comments:
File cc/input/scroll_customization.h:
Patch Set #18, Line 16:
ScrollCustomizationCan we name this ScrollCustomizationEnabledDirections, or similar?
Something that's a bit more specific?
Why are we using integer literals instead of the constants defined above?
File third_party/WebKit/Source/core/css/properties/CSSPropertyAPIScrollCustomization.cpp:
Is there precedent for using a *& here?
In general, we use pointers for out parameters, even when references work.
From the style guide:
"Input parameters are usually values or const references, while output and input/output parameters will be pointers to non-const."Patch Set #18, Line 21:
pan_xWill !pan_x ever be true?
To view, visit change 590497. To unsubscribe, or for help writing mail filters, visit settings.
Ehsan Karamad (Gerrit)
Patch Set 18:
(4 comments)
Can we add some unit tests for this?
Thanks Tim and Majid for the reviews. PTAL.
tdresser@: I assumed the unit tests you were suggesting were for CSS parsing. Also not sure if the tests I added are in the correct spot.
6 comments:
File cc/input/scroll_customization.h:
Patch Set #18, Line 16:
nLeft = 0x1,Can we name this ScrollCustomizationEnabledDirections, or similar? […]
Sure. The suggestion sounds good (a bit too long though). So I shortened the constants' names inside.
Also, the property is now 'scroll-customization' which is leading to style()->ScrollCustomization() for getter. Should I change the property name accordingly? Say, scroll-customization-direction or *-enabled-direction?
Why are we using integer literals instead of the constants defined above?
I am not sure how I could handle the combination of different properties then, such as line 65 for kScrollCustomizationPanLeft | kScrollCustomizationPanUp.
File cc/input/scroll_customization.h:
nit: please remove cmath and string includes. There are not used.
Done
File third_party/WebKit/Source/core/css/parser/CSSPropertyParserTest.cpp:
Patch Set #19, Line 282:
const CSSValue* value = CSSParser::ParseSingleValue(Putting "pan-left pan-right" will cause |value| to be nullptr. This is because
the way CSSPropertyAPIScrollCustomization.cpp implements parsing expects only one item for x-direction. But I am not sure if this is correct (actually no idea how it should be treated).Is this fine, or a correct parsing will convert something like "pan-left pan-right pan-up pan-up " to "pan-x pan-up"?
File third_party/WebKit/Source/core/css/properties/CSSPropertyAPIScrollCustomization.cpp:
Is there precedent for using a *& here?
Not really. Maybe it makes the statement inside if nicer (!pan_x instead of !*pan_x maybe?).
I did copy this from *TouchAction*.
In general, we use pointers for out parameters, even when references work.
From the style guide:
"Input parameters are usually values or const references, while output and input/output parameters will be pointers to non-const."
Thanks for the link. Agreed. Changing *& to **.
Patch Set #18, Line 21:
*pan_
Will !pan_x ever be true?
Looking at the call on line 48 I believe |pan_x| is nullptr. It may remain nullptr in the second call as well.
To view, visit change 590497. To unsubscribe, or for help writing mail filters, visit settings.
Timothy Dresser (Gerrit)
Sorry for the delay...
5 comments:
Patch Set #18, Line 16:
nLeft = 0x1,
Sure. The suggestion sounds good (a bit too long though). […]
Hmm, this feels like a decision that will be bikeshed at specification time. I don't care either way at this point.
I am not sure how I could handle the combination of different properties then, such as line 65 for k […]
If you make the bitwise operations constexpr, can you include them in the case statement? (I'm not sure)
Patch Set #19, Line 148:
thenableIs there such a thing as a non-thenable promise?
File third_party/WebKit/Source/core/css/parser/CSSPropertyParserTest.cpp:
Patch Set #19, Line 282:
const CSSValue* value = CSSParser::ParseSingleValue(
Putting "pan-left pan-right" will cause |value| to be nullptr. This is because […]
We should probably do whatever touch-action does here.
File third_party/WebKit/Source/core/dom/Element.cpp:
Patch Set #19, Line 179:
HasScrollCustomizationForGestureScrollState doesn't represent an arbitrary gesture.
Perhaps HasScrollCustomizationForGesture->IsScrollCustomized
To view, visit change 590497. To unsubscribe, or for help writing mail filters, visit settings.
Ehsan Karamad (Gerrit)
Thanks! PTAL.
I also think we might need to relocate the layout test file to some other folder.
5 comments:
Patch Set #18, Line 16:
nLeft = 0x1,
Hmm, this feels like a decision that will be bikeshed at specification time. […]
Ack
If you make the bitwise operations constexpr, can you include them in the case statement? (I'm not s […]
It works! Thanks.
Patch Set #19, Line 148:
Promise.
Is there such a thing as a non-thenable promise?
Hmm..I believe the answer is no...so all "Promise"'s are thenable as in they have "then" property [1].
[1] http://www.ecma-international.org/ecma-262/6.0/#sec-promise.prototype.then
File third_party/WebKit/Source/core/css/parser/CSSPropertyParserTest.cpp:
Patch Set #19, Line 282:
const CSSValue* value = CSSParser::ParseSingleValue(
We should probably do whatever touch-action does here.
Then looking at ConsumePan() in CSSPropertyAPITouchAction.cpp, AFAICT, this is the same [1].
In other words we only allow one item per x and one item per y coordinates.
[1] https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/css/properties/CSSPropertyAPITouchAction.cpp?rcl=a3322f51ec07d17e014a0938a78f87ab418d8273&l=14
File third_party/WebKit/Source/core/dom/Element.cpp:
ScrollState doesn't represent an arbitrary gesture. […]
Ack
To view, visit change 590497. To unsubscribe, or for help writing mail filters, visit settings.
Timothy Dresser (Gerrit)
1 comment:
Patch Set #19, Line 148:
Promise.
Hmm..I believe the answer is no... […]
Sorry, I didn't really say what I meant...
I think this is redundant - let's just say "returns a Promise".
To view, visit change 590497. To unsubscribe, or for help writing mail filters, visit settings.
Ehsan Karamad (Gerrit)
Thanks! PTAL.
1 comment:
Sorry, I didn't really say what I meant... […]
Done
To view, visit change 590497. To unsubscribe, or for help writing mail filters, visit settings.
Timothy Dresser (Gerrit)
LGTM, thanks.
Patch set 22:Code-Review +1
Ehsan Karamad (Gerrit)
Thanks for the reviews!
Adding owner reviewers:
vollick@ for cc/
boken@ for blink/
PTAL.
David Bokan (Gerrit)
This seems a little strange to me (why would an author install a handler if they don't want it called?) but I've missed all the discussion so I'm probably missing the point. Could you point me to any explainer or web discussions that have been happening here?
Also, I can stamp (once ready) for the scrolling bits in Blink but I don't really understand anything in core/css so you'll probably want to find a reviewer that can vouch for correctness there.
2 comments:
Patch Set #20, Line 10:
scroll customization unnecessary calls to apply/distributeScroll handlesnit: scroll-customization, unnecessary calls to apply/distributeScroll handlers
File third_party/WebKit/Source/core/dom/Element.cpp:
Patch Set #20, Line 731:
!has_scroll_customization_for_gestureSo if I'm understanding this API correctly, unless the author sets a `scroll-customization` value, it'll default to None in which case we won't call any of the installed handlers. Is that correct?
I suspect in its current form, this will break viewport actions. Overscroll glow and URL bar movement occur through an ApplyScroll installed by Blink on the HTML element (and changeable by document.rootScroller). You'd need to either apply an exception here for Element that is the TopDocumentRootScrollerController::GlobalRootScroller or modify TDRSC to also set this property correctly.
Have you run this through the try bots? I'd be interested to know if they miss this since that'd imply lacking test coverage.
To view, visit change 590497. To unsubscribe, or for help writing mail filters, visit settings.
Ehsan Karamad (Gerrit)
Ehsan Karamad uploaded patch set #23 to this change.
Introduce ScrollCustomization Property
This CL introduces the CSS property, 'scroll-customization'. Through
modifying scroll-customization, unnecessary calls to apply/distributeScroll handles
can be avoided.
This CL implements the logic inside blinks main thread. If an scrollable node
has customized scrolling handlers, it can further specify which class
of gestures it is interested for custom scrolling.
The list of valid values for customization includes 'auto', 'pan-x',
'pan-left', 'pan-right', 'pan-y', 'pan-up', and 'pan-down'.
This CL will eventually be followed up by changes to the compositor layer
so that for nodes with custom scroll, the scroll events corresponding
not allowed through scroll customization do not get sent to the main thread.
Bug: 765326
Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel
Change-Id: Ia54971b5535cb0b841682d242ef90761975f8c81
---
M cc/BUILD.gn
A cc/input/scroll_customization.cc
A cc/input/scroll_customization.h
A third_party/WebKit/LayoutTests/virtual/scroll_customization/fast/scroll-behavior/scroll-customization-property.html
M third_party/WebKit/Source/core/BUILD.gn
M third_party/WebKit/Source/core/css/BUILD.gn
M third_party/WebKit/Source/core/css/CSSPrimitiveValueMappings.h
M third_party/WebKit/Source/core/css/CSSProperties.json5
M third_party/WebKit/Source/core/css/CSSValueKeywords.json5
M third_party/WebKit/Source/core/css/ComputedStyleCSSValueMapping.cpp
M third_party/WebKit/Source/core/css/parser/CSSPropertyParserTest.cpp
A third_party/WebKit/Source/core/css/properties/CSSPropertyAPIScrollCustomization.cpp
M third_party/WebKit/Source/core/dom/Element.cpp
M third_party/WebKit/Source/core/frame/UseCounter.cpp
M third_party/WebKit/Source/core/page/scrolling/ScrollState.h
M third_party/WebKit/Source/devtools/front_end/sdk/CSSMetadata.js
M third_party/WebKit/Source/platform/BUILD.gn
A third_party/WebKit/Source/platform/scroll/ScrollCustomization.cpp
A third_party/WebKit/Source/platform/scroll/ScrollCustomization.h
M tools/metrics/histograms/enums.xml
20 files changed, 579 insertions(+), 6 deletions(-)
To view, visit change 590497. To unsubscribe, or for help writing mail filters, visit settings.
Ehsan Karamad (Gerrit)
Thanks for reviews! PTAL.
There seems to be a scroll customization test which is still failing. I need to investigate that. I also left two comments regarding how we could consider scroll-customization in some cases (need feedback), Thanks!
4 comments:
Patch Set #20, Line 10:
modifying scroll-customization, unnecessary calls to apply/distributeScrnit: scroll-customization, unnecessary calls to apply/distributeScroll handlers
Thanks. Done.
File third_party/WebKit/Source/core/dom/Element.cpp:
Patch Set #20, Line 731:
llStateCallback* callback =So if I'm understanding this API correctly, unless the author sets a `scroll-customization` value, i […]
I ran the trybots based on this (old) patch. Couple of tests failed mostly due to rootScroller issue you mentioned.
In the most recent patch I am ignoring customization if the element is GlobalRootScroller().
File third_party/WebKit/Source/core/dom/Element.cpp:
Patch Set #26, Line 196:
if (direction == ScrollCustomizationEnabledDirection::kNone) {This is not correct and I am thinking about a more rigorous less hacky solution. The issue I am observing is that sometimes when |scroll_state.isBeginning/Ending()| is true the deltas are 0.00 which makes it impossible to determine the direction.
Supposedly if the isBeginning() case can be determined, we 'could' hold a state here, say |element_in_custom_scroll_| and return true when the element is in scroll and the scroll state isEnding() is true (regardless of the direction).
Patch Set #26, Line 745:
!RuntimeEnabledFeatures::ScrollCustomizationEnabled() ||Is there any scenario where applyScroll or distributeScroll exist without ScrollCustomizationEnabled()? If so then this seems to be necessary.
To view, visit change 590497. To unsubscribe, or for help writing mail filters, visit settings.
David Bokan (Gerrit)
Thanks, this looks better but I'll wait until you figure out the breaking test.
4 comments:
Patch Set #26, Line 10:
handleshate to beat a dead horse, but this should be "handlers" rather than "handles", right?
File third_party/WebKit/Source/core/dom/Element.cpp:
Patch Set #26, Line 196:
if (direction == ScrollCustomizationEnabledDirection::kNone) {
This is not correct and I am thinking about a more rigorous less hacky solution. […]
+sahel@ has some insight into when we don't get direction in Begin and had to make this work for latching. She might be able to shed some light on how to fix this.
Patch Set #26, Line 745:
!RuntimeEnabledFeatures::ScrollCustomizationEnabled() ||
Is there any scenario where applyScroll or distributeScroll exist without ScrollCustomizationEnabled […]
Yes, the viewport sets the ViewportScrollCallback on the <html> Element as the applyScroll callback, even if ScrollCustomization isn't enabled (i.e. the web-facing API).
GetDocument()
.GetPage()
->GlobalRootScrollerController()
.GlobaPlease add this as an overload of RootScrollerUtil::IsGlobal and use that. It'll make the code here more readable and it'll help me keep track of all the places we have exceptions for RootScroller as I move stuff around.
To view, visit change 590497. To unsubscribe, or for help writing mail filters, visit settings.
Ehsan Karamad (Gerrit)
Ehsan Karamad uploaded patch set #27 to this change.
Introduce ScrollCustomization Property
This CL introduces the CSS property, 'scroll-customization'. Through
modifying scroll-customization, unnecessary calls to apply/distributeScroll
handlers can be avoided.
This CL implements the logic inside blinks main thread. If an scrollable node
has customized scrolling handlers, it can further specify which class
of gestures it is interested for custom scrolling.
The list of valid values for customization includes 'auto', 'pan-x',
'pan-left', 'pan-right', 'pan-y', 'pan-up', and 'pan-down'.
This CL will eventually be followed up by changes to the compositor layer
so that for nodes with custom scroll, the scroll events corresponding
not allowed through scroll customization do not get sent to the main thread.
Bug: 765326
Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel
Change-Id: Ia54971b5535cb0b841682d242ef90761975f8c81
---
M cc/BUILD.gn
A cc/input/scroll_customization.cc
A cc/input/scroll_customization.h
M third_party/WebKit/LayoutTests/fast/scroll-behavior/scroll-customization/scrollstate-distribute-to-scroll-chain-descendant.html
M third_party/WebKit/LayoutTests/fast/scroll-behavior/scroll-customization/touch-scroll-customization.html
A third_party/WebKit/LayoutTests/virtual/scroll_customization/fast/scroll-behavior/scroll-customization-property.html
M third_party/WebKit/Source/core/BUILD.gn
M third_party/WebKit/Source/core/css/BUILD.gn
M third_party/WebKit/Source/core/css/CSSPrimitiveValueMappings.h
M third_party/WebKit/Source/core/css/CSSProperties.json5
M third_party/WebKit/Source/core/css/CSSValueKeywords.json5
M third_party/WebKit/Source/core/css/ComputedStyleCSSValueMapping.cpp
M third_party/WebKit/Source/core/css/parser/CSSPropertyParserTest.cpp
A third_party/WebKit/Source/core/css/properties/CSSPropertyAPIScrollCustomization.cpp
M third_party/WebKit/Source/core/dom/Element.cpp
M third_party/WebKit/Source/core/frame/UseCounter.cpp
M third_party/WebKit/Source/core/page/scrolling/ScrollState.h
M third_party/WebKit/Source/devtools/front_end/sdk/CSSMetadata.js
M third_party/WebKit/Source/platform/BUILD.gn
A third_party/WebKit/Source/platform/scroll/ScrollCustomization.cpp
A third_party/WebKit/Source/platform/scroll/ScrollCustomization.h
M tools/metrics/histograms/enums.xml
22 files changed, 606 insertions(+), 6 deletions(-)
To view, visit change 590497. To unsubscribe, or for help writing mail filters, visit settings.
Ehsan Karamad (Gerrit)
Ehsan Karamad uploaded patch set #28 to this change.
Introduce ScrollCustomization Property
This CL introduces the CSS property, 'scroll-customization'. Through
modifying scroll-customization, unnecessary calls to apply/distributeScroll
handlers can be avoided.
This CL implements the logic inside blink's main thread. If an scrollable
node has customized scrolling handlers, it can further specify which class
of gestures it is interested in for custom scrolling.
The list of valid values for customization includes 'auto', 'pan-x',
'pan-left', 'pan-right', 'pan-y', 'pan-up', and 'pan-down', and 'none'.
The default value is 'none' (any node interested in customized scroll
handlers should specify that).
For RootScroller we ignore customized scroll as it will regress certain
features which depend on global RootScroller.
This CL will eventually be followed up by changes to the compositor layer
so that for nodes with custom scroll, the scroll events corresponding
not allowed through scroll customization do not get sent to the main thread.
Bug: 765326
Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel
Change-Id: Ia54971b5535cb0b841682d242ef90761975f8c81
---
M cc/BUILD.gn
A cc/input/scroll_customization.cc
A cc/input/scroll_customization.h
M third_party/WebKit/LayoutTests/fast/scroll-behavior/scroll-customization/scrollstate-distribute-to-scroll-chain-descendant.html
M third_party/WebKit/LayoutTests/fast/scroll-behavior/scroll-customization/touch-scroll-customization.html
A third_party/WebKit/LayoutTests/virtual/scroll_customization/fast/scroll-behavior/scroll-customization-property.html
M third_party/WebKit/Source/core/BUILD.gn
M third_party/WebKit/Source/core/css/BUILD.gn
M third_party/WebKit/Source/core/css/CSSPrimitiveValueMappings.h
M third_party/WebKit/Source/core/css/CSSProperties.json5
M third_party/WebKit/Source/core/css/CSSValueKeywords.json5
M third_party/WebKit/Source/core/css/ComputedStyleCSSValueMapping.cpp
M third_party/WebKit/Source/core/css/parser/CSSPropertyParserTest.cpp
A third_party/WebKit/Source/core/css/properties/CSSPropertyAPIScrollCustomization.cpp
M third_party/WebKit/Source/core/dom/Element.cpp
M third_party/WebKit/Source/core/frame/UseCounter.cpp
M third_party/WebKit/Source/core/page/scrolling/ScrollState.h
M third_party/WebKit/Source/devtools/front_end/sdk/CSSMetadata.js
M third_party/WebKit/Source/platform/BUILD.gn
A third_party/WebKit/Source/platform/scroll/ScrollCustomization.cpp
A third_party/WebKit/Source/platform/scroll/ScrollCustomization.h
M tools/metrics/histograms/enums.xml
22 files changed, 606 insertions(+), 6 deletions(-)
To view, visit change 590497. To unsubscribe, or for help writing mail filters, visit settings.
Ehsan Karamad (Gerrit)
Thanks David. I am still poking at the failing test:
'scrollstate-distribute-to-scroll-chain-descendant.html' where there seems to be
a crash on around line 39: "scrollState.distributeToScrollChainDescendant()".
Also I will wait for feedback on eventSender.* as well as the (0, 0) case for deltas in scrollBegin/end. Thanks!
4 comments:
hate to beat a dead horse, but this should be "handlers" rather than "handles", right?
Thanks! :) I added more details to the message.
File third_party/WebKit/Source/core/dom/Element.cpp:
Patch Set #26, Line 196:
if (direction == ScrollCustomizationEnabledDirection::kNone) {
+sahel@ has some insight into when we don't get direction in Begin and had to make this work for lat […]
This is one (or the?) case when this happens; specifically in the context of "touch-scroll-customization.html" test which relies on eventSender.gestureScrollBegin.
If this is possibly the only reason behind (0,0) deltas, I believe we should fix the event synthesizing code to setup some gesture direction or perhaps even pass it as parameters to eventSender.gestureScrollBegin. The only issue is that I guess eventSender stuff are deprecated(?).
Patch Set #26, Line 745:
!RuntimeEnabledFeatures::ScrollCustomizationEnabled() ||
Yes, the viewport sets the ViewportScrollCallback on the <html> Element as the applyScroll callback, […]
Ack
GetDocument()
.GetPage()
->GlobalRootScrollerController()
.Globa
Please add this as an overload of RootScrollerUtil::IsGlobal and use that. […]
Yes good idea. Done!
To view, visit change 590497. To unsubscribe, or for help writing mail filters, visit settings.
Alexandre Elias (Gerrit)
1 comment:
Patch Set #29, Line 57:
var testCases = [https://developer.mozilla.org/en-US/docs/Web/CSS/touch-action says that, for touch-action, '"pan-left pan-right" is invalid since "pan-x" is simpler'. For consistency, this new CSS property should behave the same. Could you make sure "pan-left pan-right" are invalid in the same way as touch-action and add a test case verifying the exact failure behavior?
To view, visit change 590497. To unsubscribe, or for help writing mail filters, visit settings.
Ehsan Karamad (Gerrit)
Thanks. PTAL at the new tests. (still investigating the test failure).
1 comment:
Patch Set #29, Line 57:
var testCases = [
https://developer.mozilla. […]
Thanks. I think the way the property parsing is implemented is similar to that of 'touch-action'. I added new test cases here as well as unit tests to CSSPropertyParserTest.
To view, visit change 590497. To unsubscribe, or for help writing mail filters, visit settings.
Ehsan Karamad (Gerrit)
Tests fixed with two questions for tdresser@ who was the test author.
PTAL.
2 comments:
Patch Set #32, Line 30:
element.style.scrollCustomization = 'auto';Are these changes OK, or do we really have to support unattached elements?
File third_party/WebKit/Source/core/dom/Element.cpp:
Patch Set #32, Line 183:
if (!box)Does it make sense to customize scroll for an element without a box?
To view, visit change 590497. To unsubscribe, or for help writing mail filters, visit settings.
Timothy Dresser (Gerrit)
3 comments:
Patch Set #32, Line 30:
element.style.scrollCustomization = 'auto';Are these changes OK, or do we really have to support unattached elements?
Most styling can be applied before an element is attached, right?
I think we probably want to support unattached elements.
Patch Set #32, Line 32:
updateLayoutIgnorePendingStylesheetsAndRunPostLayoutTasksWhy is this needed?
File third_party/WebKit/Source/core/dom/Element.cpp:
Patch Set #32, Line 183:
if (!box)Does it make sense to customize scroll for an element without a box?
I think so, in the same way as it makes sense to set a style for an object that isn't attached yet.
To view, visit change 590497. To unsubscribe, or for help writing mail filters, visit settings.
Ian Vollick (Gerrit)
1 comment:
File cc/input/scroll_customization.h:
Patch Set #32, Line 50:
inline const char* ScrollCustomizationToString(Is this used?
To view, visit change 590497. To unsubscribe, or for help writing mail filters, visit settings.
Majid Valipour (Gerrit)
1 comment:
Patch Set #32, Line 183:
if (!box)
I think so, in the same way as it makes sense to set a style for an object that isn't attached yet.
Can you actually ave style for un-attached object?
Both these return empty string "":
> getComputedStyle(document.createElement("div")).overflow
> var el = document.createElement("div"); el.style.overflow="visible"; getComputedStyle(el).overflowTo me this test feels a bit artificial given that it creates the
scroll chain using an internal method. In practice, will we ever
end up customizing scroll on an unattached element? Those element
are never target of a scroll gesture.Given this I am not sure if we should go out of our way to support
this property on un-attached elements.
To view, visit change 590497. To unsubscribe, or for help writing mail filters, visit settings.
Timothy Dresser (Gerrit)
Patch Set 32:
(1 comment)
See an example of why this matters here:
https://output.jsbin.com/rivawug/quiet
If I'm creating an element, I can style it before or after I attach it. You're right that it doesn't matter until it's attached, but it's still important to allow doing things in this order.
In the same way, I should be able to customize scroll on an element before or after it's attached.
Patch set 32:-Code-Review
Ehsan Karamad (Gerrit)
Patch Set 32: -Code-Review
Patch Set 32:
(1 comment)
See an example of why this matters here:
https://output.jsbin.com/rivawug/quietIf I'm creating an element, I can style it before or after I attach it. You're right that it doesn't matter until it's attached, but it's still important to allow doing things in this order.
In the same way, I should be able to customize scroll on an element before or after it's attached.
I think the issue is mostly "calling" the handlers on unattached elements. I do agree we can setup properties and handlers before attaching - but the handlers should not get called before being attached. Specifically scrolling which is input related.
Timothy Dresser (Gerrit)
Patch Set 32:
Patch Set 32: -Code-Review
Patch Set 32:
(1 comment)
See an example of why this matters here:
https://output.jsbin.com/rivawug/quietIf I'm creating an element, I can style it before or after I attach it. You're right that it doesn't matter until it's attached, but it's still important to allow doing things in this order.
In the same way, I should be able to customize scroll on an element before or after it's attached.
I think the issue is mostly "calling" the handlers on unattached elements. I do agree we can setup properties and handlers before attaching - but the handlers should not get called before being attached. Specifically scrolling which is input related.
I agree we shouldn't have handlers called before we're attached. You're right, I was incorrectly thinking about handler registration, and others were correctly thinking about handler execution. Sorry about that!
Ehsan Karamad (Gerrit)
Thanks. PTAL.
Also adding rbyres@ for 'event_sender.cc' changes. PTAL.
4 comments:
File cc/input/scroll_customization.h:
Patch Set #32, Line 50:
// Returns the scroll customization which is compatible with the providedIs this used?
Opps :P. I kind of copied code from TouchAction and did not really pay attention to method not necessarily being needed. Thanks for catching it.
Patch Set #32, Line 30:
element.style.scrollCustomization = 'auto';
Most styling can be applied before an element is attached, right? […]
I guess as things are right now we can set the 'scroll-customization' style for unattached elements. So this should be fine.
Patch Set #32, Line 32:
updateLayoutIgnorePendingStylesheetsAndRunPostLayoutTasksWhy is this needed?
Without this line the test fails. I believe the style is not setup properly. If we remove this line and instead load the tests after window.onload they pass. But dropping this line does not setup the style properly.
File third_party/WebKit/Source/core/dom/Element.cpp:
Patch Set #26, Line 196:
scroll_state.effectiveDeltaY());This is one (or the?) case when this happens; specifically in the context of "touch-scroll-customiza […]
I changed eventSender's API to support providing deltas for gestureScrollBegin and gestureScrollEnd.
Alternatively, I could modify the test touch-scroll-customization.html which was failing due to this root cause and migrate it to using gpuBenchmarking.pointerActionSequence API (the same as the newly added test scroll-customization-property.html). But I think the fact that eventSender.gestureScrollBegin forces the deltas to 0 is not correct.
The tests should be fine now and lines 196-198 are removed.
To view, visit change 590497. To unsubscribe, or for help writing mail filters, visit settings.
David Bokan (Gerrit)
12 comments:
File cc/input/scroll_customization.h:
Patch Set #33, Line 23:
kMax = (1 << 4) - 1If this is unused, please remove it.
File content/shell/test_runner/event_sender.cc:
Add `// fallthrough` to make it clear the omitted `break` is intentional (here and for kGestureScrollBegin)
File third_party/WebKit/LayoutTests/LeakExpectations:
Patch Set #33, Line 14:
crbug.com/410974 virtual/scroll_customization/fast/scroll-behavior/scroll-customization/scroll-customization-property.html [ Leak ]Why does the test leak? We shouldn't commit a leaky test/CL.
File third_party/WebKit/LayoutTests/TestExpectations:
Patch Set #33, Line 650:
crbug.com/410974 fast/scroll-behavior/scroll-customization/scroll-customization-property.html [ Pass Failure ]Why is it flaky?
tml>
<headBlink LayoutTest style is to omit <html> <head> and <body> tags. See https://chromium.googlesource.com/chromium/src/+/master/docs/testing/writing_layout_tests.md
Patch Set #33, Line 27:
height: 3000px;Does the body have to be scrollable? And if so, should it be horizontally scrollable too?
Patch Set #33, Line 42:
internalsCheck for the internals object before calling.
Patch Set #33, Line 145:
didCallScrollCustomizationHandlersThis should be didCallDistributeScrollHandler
Patch Set #33, Line 206:
testCases.forEach(runTestCase);Should only do this if we have chrome.gpuBenchmarking (generally we make the test loadable and testable manually but it'd be a little difficult in this case...)
Patch Set #32, Line 32:
updateLayoutIgnorePendingStylesheetsAndRunPostLayoutTasksWithout this line the test fails. I believe the style is not setup properly. […]
The <script> is inside the <body> tag so body isn't yet fulky parsed. The test shouldn't have <head> or <body> tags anyway so you can remove it. I prefer to make tests run in onload anyway so I'd go ahead and put this all inside a load handler.
File third_party/WebKit/Source/core/dom/Element.cpp:
Patch Set #33, Line 648:
!RuntimeEnabledFeatures::ScrollCustomizationEnabled()This is separate from whether we have a customization (i.e. if the REF is off, has_scroll_customization_for_gesture is true which is misleading). It would be more clear to include it in the if below.
File tools/metrics/histograms/enums.xml:
Patch Set #33, Line 26051:
<int value="556" label="line-break"/>Is this change intentional?
Ehsan Karamad (Gerrit)
Sorry for the long delay in sending the new CL. The design around filtering scroll events changed a bit (following discussions with David and Majid).
Ptal.
12 comments:
If this is unused, please remove it.
Ack
File content/shell/test_runner/event_sender.cc:
Add `// fallthrough` to make it clear the omitted `break` is intentional (here and for kGestureScrol […]
Ended up undoing the change here. The logic in Element.cpp allows all scroll events when customization is set to kAuto. The change here would have been useful (although I should have changed the scroll_being instead of scroll_update) but since eventSender is deprecated and we are also adding web-platform-tests then it is no longer needed.
File third_party/WebKit/LayoutTests/LeakExpectations:
Patch Set #33, Line 14:
crbug.com/410974 virtual/scroll_customization/fast/scroll-behavior/scroll-customization/scroll-customization-property.html [ Leak ]Why does the test leak? We shouldn't commit a leaky test/CL.
The test is using the ScrollCustomization API (setApplyScroll/setDistributeScroll) similarly to the other related tests such as touch-scroll-customization.html. The leak itself is not introduced in the CL and is a separate bug tracked by majidvp@. I believe the root cause is a dependency loop form Document -> handler -> element -> Document or something similar. majidvp@ should have a better idea.
File third_party/WebKit/LayoutTests/TestExpectations:
Patch Set #33, Line 650:
### Crash site: layout_ng_block_flow.ccWhy is it flaky?
I just repeated what we have done for other ScrollCustomization tests here. IIUC we have to make it pass since the real test is supposed to pass as a virtual/scroll_customization/... where --enbale-fetures=ScrollCustomization is present?
itle> Scroll Customization Property </title>
<scriBlink LayoutTest style is to omit <html> <head> and <body> tags. See https://chromium.googlesource. […]
Thanks I vaguely remembered this (perhaps you'd told me :)) but was not sure. Done.
Does the body have to be scrollable? And if so, should it be horizontally scrollable too?
I believe not. We are only interested in class="a" div and whether or not the handlers are called. Thanks for catching these. :)
Patch Set #33, Line 42:
element.d
Check for the internals object before calling.
Done...but do we need to assert it too?
Patch Set #33, Line 145:
andler = false;This should be didCallDistributeScrollHandler
Yes. Thanks.
Should only do this if we have chrome. […]
Hmm....right. I know it is not ideal but if we do not use "chrome.gpuBenchmarking" when it does not exist, then the test will fail. Does it make it better (new patch)?
The <script> is inside the <body> tag so body isn't yet fulky parsed. […]
Done.
File third_party/WebKit/Source/core/dom/Element.cpp:
This is separate from whether we have a customization (i.e. […]
Good point. How about changing this to "should_consider_scroll_customization" and then adding IsScrollCustomized(this, scroll_state) to the conditional on line 653, instead?
File tools/metrics/histograms/enums.xml:
Patch Set #33, Line 26051:
<int value="1000" label="CBErrorUnknoIs this change intentional?
No sorry. I guess bad rebase. Un-done.
To view, visit change 590497. To unsubscribe, or for help writing mail filters, visit settings.
Ehsan Karamad (Gerrit)
David Bokan (Gerrit)
Apologies for the delay.
14 comments:
File cc/input/scroll_customization.h:
Patch Set #44, Line 13:
ScrollCustomizationEnabledDirectionI don't necessarily have a strong opinion here, but IMO, if you're going to be using values as a bit field it makes more sense to just make them const int values rather than an enum. e.g. kPanRight | kPanDown isn't a valid value in your enum so it's somewhat awkward.
File third_party/WebKit/LayoutTests/LeakExpectations:
Patch Set #33, Line 14:
crbug.com/410974 virtual/scroll_customization/fast/scroll-behavior/scroll-customization/scroll-customization-property.html [ Leak ]
The test is using the ScrollCustomization API (setApplyScroll/setDistributeScroll) similarly to the […]
Ah, ok, I missed the context here.
File third_party/WebKit/LayoutTests/TestExpectations:
Patch Set #33, Line 650:
### Crash site: layout_ng_block_flow.cc
I just repeated what we have done for other ScrollCustomization tests here. […]
But it fails without the flag? If that's the case the tests should live in LayoutTests/virtual/scroll_customization so they only run with the flag. Could you find out why these are all here and if the reason is just because of the flag behavior we can move them in a separate patch. Otherwise, please add a comment above explaining why all scroll-customization tests are allowed to be flaky.
Patch Set #33, Line 42:
element.dDone... […]
No, we usually like that layout tests can be open and run in a live browser. In cases where you need internals, at the least we just don't want JS errors.
Hmm....right. I know it is not ideal but if we do not use "chrome. […]
I replied on the new patch.
test(function() {
assert_true('ScrollState' in window, "'ScrollState' in window");
}, 'These tests only work with scroll customization enabled.')Lets add asserts here for internals and gpuBenchmarking that output a message to make it clear that this test only works in content shell with these enabled. You can then remove the checks below I think (a failing assert throws IIRC).
Patch Set #44, Line 56:
// [scroll customization, gesture, scroll customization handlerYou can shorten the distance between columns to make this comment fit in 80 cols
Patch Set #44, Line 180:
deltaX * 2), y : (deltaY * 2)Is this realistic? Does an Up also include some delta that isn't represented in a Move? Would it hurt to just have pointerUp at (deltaX, deltaY)?
Patch Set #44, Line 191:
element.scrollCustomizationshouldn't this be `element.style.scrollCustomization`?
Patch Set #44, Line 196:
Invalid behavior for gesture input "${testCase[1]}" and scroll customization "${testCase[0]}You already specify testCase[0] and testCase[1] in the test()'s description. This should probably state whether the case expected customization callbacks to be called and whether they were or not.
File third_party/WebKit/Source/core/dom/Element.cpp:
Good point. […]
sgtm.
File third_party/WebKit/Source/core/dom/Element.cpp:
Patch Set #44, Line 620:
!GetScrollCustomizationCallbacks().InScrollPhase(this);This "in scroll phase" state is new. What is the consequence of not having it? It seems that we flip this bit on everything in the scroll chain at scroll begin - how can we get here if we're not in the scroll chain and thus have the bit flipped?
scroll_customization != ScrollCustomizationEnabledDirection::kNone &&
((scroll_customization == ScrollCustomizationEnabledDirection::kAuto)You shouldn't need to separate the None case explicitly here - it'll be handled correctly by the bitfield check.
Auto would as well except for the case where deltaX and deltaY are both < the epsilon. Do we really want to start a scroll phase if the delta hint has no direction?
File third_party/WebKit/Source/core/input/ScrollManager.cpp:
Patch Set #44, Line 724:
static_cast<Element*>use ToElement()
Ehsan Karamad (Gerrit)
Thanks David and sorry it took this long. I noticed some fundamental issues with the test but now it is fixed. I tried to pinpoint the important issues and changes. PTAL.
14 comments:
Patch Set #44, Line 13:
ScrollCustomizationEnabledDirection
I don't necessarily have a strong opinion here, but IMO, if you're going to be using values as a bit […]
I like changing it to int type. But first to understand your suggestion properly, the "type_name" should become uint8_t and then keep these values as constants as:
namespace cc {
const uint8_t kScrollCustomizationEnabledDirectionNone = 0;
...
}or maybe in its own namespace or a struct?
File third_party/WebKit/LayoutTests/TestExpectations:
Patch Set #33, Line 650:
crbug.com/635619 virtual/layout_ng/external/wpt/css/CSS2/floats-clear/floats-130.xht [ Skip ]
But it fails without the flag? If that's the case the tests should live in LayoutTests/virtual/scroll_customization so they only run with the flag. Could you find out why these are all here and if the reason is just because of the flag behavior we can move them in a separate patch. Otherwise, please add a comment above explaining why all scroll-customization tests are allowed to be flaky.
From what I am understanding in the document the way things are is apparently how virtual tests are added:
https://chromium.googlesource.com/chromium/src/+/master/docs/testing/layout_tests.So basically if we do not expect the "virtual" test to pass without the feature we should mark it so here.
I also do not find any actual tests in LayoutTests/virtual? I am not sure if we should move the test there. When we run run-webkit-tests virtual/scroll_customization/fast/scroll-behavior the script runs all the tests in LayoutTests/fast/scroll-behavior with the flag passed.
Patch Set #33, Line 42:
assert_No, we usually like that layout tests can be open and run in a live browser. […]
Ack
top: 1000px;
}Lets add asserts here for internals and gpuBenchmarking that output a message to make it clear that […]
Done
Patch Set #44, Line 56:
var testCases = [
You can shorten the distance between columns to make this comment fit in 80 cols
Done. Thanks for the suggestion.
Is this realistic? Does an Up also include some delta that isn't represented in a Move? Would it hur […]
Changed. Thanks! Also, I am now using eventSender. gpuBenchmarking seems to have issues with ScrollCustomization (crbug.com/799973).
Patch Set #44, Line 191:
chrome.gpuBenchmarking.poinshouldn't this be `element.style. […]
Yes. This is wrong...and yet it passed because they never made it to the asserts. I should have put in some promise/async tests instead...which I am dong now :P.
(the changes in made to scrollstate-distribute-...html were incorrect as well).In the newest patch all the tests are promise_test and I can guarantee that they really run this time :).
Patch Set #44, Line 196:
e: 'pointerMove', x: (startingX + deltaX),You already specify testCase[0] and testCase[1] in the test()'s description. […]
Done
Patch Set #48, Line 35:
<p id="p1"> Test </p>This is my way of making sure the <div> can actually scroll in all 4 directions (by scrolling p1 into view). Open to any suggestions for a nicer way of achieving this.
Patch Set #48, Line 195:
{ name: 'pause', duration: pauseSeconds},Due to the pauses here, the tests takes abive 10 second. Hence, I added this as a SlowTest,
Patch Set #48, Line 72:
propagatesCorrectlyTest.done();I am basically making sure the test runs after window.onload event. So I broke it up like this. The logic is intact.
File third_party/WebKit/Source/core/dom/Element.cpp:
Patch Set #44, Line 620:
!GetScrollCustomizationCallbacks().InScrollPhase(this);This "in scroll phase" state is new.
Yes. We now decide whether a scroll direction matches the property during GestureScrollBegin. If so |InScrollPhase(this)| returns true. This is also done for each scroll phase and for all the elements inside the scroll chain. The InScrollPhase() will return false after GestureScrollEnd.
What is the consequence of not having it?
The callbacks will not get invoked.
It seems that we flip this bit on everything in the scroll chain at scroll begin
Yes. The bit is flipped for all elements inside the chain whose customization matches the scrollBegin's direction.
how can we get here if we're not in the scroll chain and thus have the bit flipped?
I don't think we get here if not on scroll chain already.
turn GetElementRareData()->IntersectionObserverData();
return nullptr;
You shouldn't need to separate the None case explicitly here - it'll be handled correctly by the bitfield check.
Yes agreed. I ended up simplifying this whole method quite a bit; starting with the fact that we do not actually need |scroll_state|. I am passing the direction now. This would save computing it every time for each node on the scroll chain.
Auto would as well except for the case where deltaX and deltaY are both < the epsilon. Do we really want to start a scroll phase if the delta hint has no direction?
This was intentional originally. Please look at scroll_customization.cc and the new definition of GetScrollCustomizationForDirection().
Essentially, there is a bunch of places where delta hints are 0 -- most important of all is 'touch-scroll-customization.html' which uses eventSender.gestureScrollBegin (it does not take deltas).
At first I tried changing eventSender so that delta hints could be passed as arguments. This revealed another problem however: in ScrollManager::CanScroll we currently return true 'true' when hints are zero. Because of that, all the <div>'s in touch-scroll-customization.html end up on the scroll chain automatically (while they shouldn't have normally since their scrollTop and scrollLeft are not properly setup for the purpose). I talked to sahel@ about this and apparently this problem exists in some "20 or so layout tests". So removal of the early return code in CanScroll() should perhaps be done in another CL. Also, to modify 'touch-scroll-customization.html' such that it won't rely on deltas = 0 proved to be problematic as the tests in the file might need to be adjusted accordingly -- perhaps even moved to using gpuBenchmarking.pointerActionSequence might lead to a re-write of most of the tests.
To conclude the story, I now duplicated the hack (:P) in scroll_customization.cc where we decide the direction is kAuto when deltas are zero. I am happy to followup on this matter in an upcoming patch and revise touch-scroll-customization.html for that matter.
File third_party/WebKit/Source/core/input/ScrollManager.cpp:
Patch Set #44, Line 724:
eEvent(targeted_eventuse ToElement()
Done. Thanks!
To view, visit change 590497. To unsubscribe, or for help writing mail filters, visit settings.
Ehsan Karamad (Gerrit)
Ehsan Karamad (Gerrit)
This change is ready for review.
David Bokan (Gerrit)
12 comments:
Patch Set #44, Line 13:
ScrollCustomizationEnabledDirection
I like changing it to int type. […]
Sure, uint8_t and putting it in a namespace sgtm. Please use the pattern:
kPanRight = 1 << 2;
kPanUp = 1 << 3;etc.
File third_party/WebKit/LayoutTests/TestExpectations:
Patch Set #33, Line 650:
crbug.com/635619 virtual/layout_ng/external/wpt/css/CSS2/floats-clear/floats-130.xht [ Skip ]
> But it fails without the flag? If that's the case the tests should live in LayoutTests/virtual/scr […]
I don't think that's good practice and I don't see that suggested in the doc (I suppose it doesn't mention LayoutTests/virtual explicitly - I suspect it may be a recent addition). However, we do have some tests in LayoutTests/virtual. I've added some recently in virtual/android/url-bar and virtual/android/rootscroller and there exist a few others. I've had good success with this approach so I don't see a reason to avoid it.
Using the approach here means doubling the amount of work the poor bots have to do for scroll-customization for no reason as well as adding confusion (is the test flaky? should it be fixed?). Absent any reason I'm missing I think we should just add/move scroll-customization tests to LayoutTests/virtual.
I believe not. We are only interested in class="a" div and whether or not the handlers are called. […]
Done
Patch Set #48, Line 35:
<p id="p1"> Test </p>
This is my way of making sure the <div> can actually scroll in all 4 directions (by scrolling p1 int […]
I would just add a <div id="spacer" style="width: 1000px; height: 1000px></div> inside. Then reset the scrollLeft and scrollTop to 300px explicitly. (or calculate it using (scrollWidth - clientWidth)/2).
Patch Set #48, Line 45:
TheseNit: gpuBenchmarking should be here too.
Patch Set #48, Line 195:
{ name: 'pause', duration: pauseSeconds},
Due to the pauses here, the tests takes abive 10 second. […]
Ack. If it's related to fling, shouldn't we only need the pause before pointerUp? Please file a bug to fix this test and block it on the gpuBenchmarking bug currently assigned to Navid.
Patch Set #48, Line 61:
These tests need to run after window.onload (document.body is required)document.body is guaranteed to be valid in every way during window.onload so a regular test should work fine but I don't really care either way (I'd just remove the comment).
Patch Set #48, Line 89:
console.log('Running first test');I'm not sure what our policy on console.log in tests is but I would remove these.
File third_party/WebKit/Source/core/dom/Element.cpp:
Patch Set #44, Line 620:
!GetScrollCustomizationCallbacks().InScrollPhase(this);
> This "in scroll phase" state is new. […]
Ok, I think I get how it works but let me confirm with an example. Lets say you have:
<div id="A"> scroll-customization: pan-x
<div id="B"> scroll-customization: pan-y
<div id="C"> scroll-customization: pan-xIf we do a horizontal scroll over C, we'll execute the handlers for C and A but not B, right?
What if we start a horizontal scroll but then switch to vertical, in that case it still won't execute the handler for B but will for A and C. Is this the intended behavior?
turn GetElementRareData()->IntersectionObserverData();
return nullptr;
> You shouldn't need to separate the None case explicitly here - it'll be handled correctly by the b […]
Ack
File third_party/WebKit/Source/core/page/scrolling/ScrollCustomizationCallbacks.cpp:
Patch Set #48, Line 50:
elementDCHECK that element isn't nullptr.
File third_party/WebKit/Source/platform/scroll/ScrollCustomization.h:
Nit: All these should now be 2018
Ehsan Karamad (Gerrit)
Thanks Daivd. PTAL.
12 comments:
Sure, uint8_t and putting it in a namespace sgtm. Please use the pattern: […]
Done. but I added the change to platform/scroll/ScrollCustomization.h/cpp and removed this file. Looking at the CL again we are not doing anything cc-related right now. The objective of having scroll-customization inside cc/ would be a future work and perhaps we could add cc/input/scroll_customization.h/cc later?
File third_party/WebKit/LayoutTests/TestExpectations:
Patch Set #33, Line 650:
crbug.com/635619 virtual/layout_ng/external/wpt/css/CSS2/positioning/abspos-inline-004.xht [ Skip ]I don't think that's good practice and I don't see that suggested in the doc (I suppose it doesn't m […]
Thanks for the detailed explanation. What I had done before is somewhat mentioned in the doc listed. But regardless, your explanation makes sense, esp. about the part about the bots. I moved the test to virtual/ and made the required modifications.
I would just add a <div id="spacer" style="width: 1000px; height: 1000px></div> inside. […]
Thanks for the suggestion! Done.
Nit: gpuBenchmarking should be here too.
Yes I forgot. Thanks!
Ack. […]
You are right. I think the this one is not necessary. I only need the one before 'pointerUp'. Thanks!
That being said, is it still considered to be a bug? i.e., having to put in a pause in between (as opposed to this just being a the need for more explicit documentation about pointerActionSequence). If so I will add the bug number here.
Patch Set #48, Line 61:
propagatesCorrectlyTest = async_test("distributeToScrollChainDescendandocument.body is guaranteed to be valid in every way during window. […]
Ack. But I was getting an exception due to document.body being nullptr before. This is change is mainly for the setup phase which has to run after window.onload. I think the tests themselves, as you said, will run after onload.
Patch Set #48, Line 89:
emptyScrollChainTestLogic();
I'm not sure what our policy on console.log in tests is but I would remove these.
Uh this is left by mistake. Thanks for catching it.
File third_party/WebKit/Source/core/dom/Element.cpp:
Ok, I think I get how it works but let me confirm with an example. Lets say you have: […]
Yes. This is the intended behavior. We only consider GestureScrollBegin. So any update until the very next GestureScrollEnd will be sent to any node whose customization matches the direction of GestureScrollBegin.
One other alternative would have been to do per-GSUpdate filtering. But that might lead to some complications. For example, what if we start vertical and then go horizontal. The scrolling for A and C will not have a GSB. Also, how call GSE for such gestures is unclear.
A third option would have been inserting GSE + GSB whenever the direction of update changes (to dissect a gesture into a series of up,down, left, and right gestures. But that would maybe break stuff (i.e., multiple gesture scroll begins before user lifts their finger).
There is some comments about this in the document. But the doc is google.com only.
majidvp@: I think we should make it available to chromium.org and add the link to the CL?
File third_party/WebKit/Source/core/input/ScrollManager.cpp:
Patch Set #50, Line 754:
scroll_customization::ScrollDirection direction =Is this against blink formatting? I ended up with presubmit errors in all such occurrences. If that is the case, then I guess the idea of 'int' inside namespace might not quite work. Maybe revert to enum or use a struct instead?
File third_party/WebKit/Source/core/page/scrolling/ScrollCustomizationCallbacks.cpp:
DCHECK that element isn't nullptr.
Done
File third_party/WebKit/Source/platform/scroll/ScrollCustomization.h:
Patch Set #50, Line 12:
using ScrollDirection = unsigned char;I get a strange compile error for "uint8_t". "unsigned char" works.
File third_party/WebKit/Source/platform/scroll/ScrollCustomization.h:
Nit: All these should now be 2018
Ack
To view, visit change 590497. To unsubscribe, or for help writing mail filters, visit settings.
David Bokan (Gerrit)
Sorry for the delay, meant to get to this yesterday but got sidetracked.
10 comments:
Done. but I added the change to platform/scroll/ScrollCustomization.h/cpp and removed this file. […]
sgtm
You are right. I think the this one is not necessary. I only need the one before 'pointerUp'. […]
It's not a bug since that's how the gesture recognizer really works. However, we should add an ability to turn of fling generation so we can make tests like this run quickly and reliably. Could you file a bug with that feature request against Navid?
Patch Set #48, Line 61:
propagatesCorrectlyTest = async_test("distributeToScrollChainDescendan
Ack. But I was getting an exception due to document.body being nullptr before. […]
Ah, yeah, since there's no displayed elements (e.g. <div>) above, any script run here will be run before <body> is created. But since you're calling it from onload I'd expect there to be a body by then. In any case, that's just FYI - looks good to me.
I am basically making sure the test runs after window.onload event. So I broke it up like this. […]
You mean during the onload event, right? My understanding is that a non-async tests are for cases where the test can complete in the load handler, like this. async_test is for tests that need to do something after load (e.g. requestAnimationFrame, event listeners, etc). It should be fairly straightforward to convert to non-async so I'd suggest trying that but if you're still having issues and it's drawing too much time this is fine too.
File third_party/WebKit/Source/core/dom/Element.cpp:
Yes. This is the intended behavior. We only consider GestureScrollBegin. […]
Thanks for explaining. Linking to the doc from CL sgtm
File third_party/WebKit/Source/core/input/ScrollManager.cpp:
Patch Set #50, Line 754:
scroll_customization::ScrollDirection direction =
Is this against blink formatting? I ended up with presubmit errors in all such occurrences. […]
I can't see why it would be, what does the error say?
File third_party/WebKit/Source/platform/scroll/ScrollCustomization.h:
Patch Set #50, Line 11:
scroll_customizationhmm...style guide says lower case but it looks out of place and most sub-namespaces in Blink code are CamelCase (couldn't find any with an underscore). I'd make this CamelCase to maintain consistency.
Patch Set #50, Line 12:
using ScrollDirection = unsigned char;I get a strange compile error for "uint8_t". "unsigned char" works.
IIRC style guide explicitly forbids relying on non uintX_t types if you need a specific width. Did you include <stdint.h>? platform/wtf/dtoa/utils.h includes that and does some defines on Windows which maybe is missing that so maybe that's better (though other examples like PODArena.h simply include stdint.h)?
Patch Set #50, Line 14:
constThese should be constexpr. Can we not just assign them in the header and remove PLATFORM_EXPORT? There's no reason these should be allocated and linked rather than just inlined.
File third_party/WebKit/Source/platform/scroll/ScrollCustomization.cpp:
Nit: Remove
Navid Zolghadr (Gerrit)
1 comment:
It's not a bug since that's how the gesture recognizer really works. […]
The bug that was filed was that when we were creating fling gesture the callback here was never called which is incorrect in any situation.
I prefer not to have that option for turning off fling though. I'm pretty sure this option will not be available in the webdriver version of these APIs and if you start using that here it wouldn't be easy to change this to use webdriver testing API.
To view, visit change 590497. To unsubscribe, or for help writing mail filters, visit settings.
David Bokan (Gerrit)
1 comment:
The bug that was filed was that when we were creating fling gesture the callback here was never call […]
It seems unreasonable to require each scroll to take at least 50ms and that won't scale. Whether or not web driver support for this is currently planned, WPT will eventually have to grapple with how to deal with this since if we were to move all our scrolling layout tests to use WebDriver in this form testing would slow to a crawl. e.g. This test takes ~5 *seconds* to run. Multiply that by the number of tests that will want to scroll.
One solution would be to write WPT with a "kScrollCoolDown" constant that browsers can tweak. If we can turn of fling generation we could ratchet that value down.
Ehsan Karamad (Gerrit)
Thanks David. PTAL.
9 comments:
It seems unreasonable to require each scroll to take at least 50ms and that won't scale. […]
Given the discussion here I will file a new bug regardless of whether or not we are adding the feature. It sure is a different issue thant 799973 which is about fling callbacks.
Ah, yeah, since there's no displayed elements (e.g. […]
Ack
Patch Set #48, Line 72:
assert_equals(elements[i].creationOrder, elements[i].calledOrder);You mean during the onload event, right? My understanding is that a non-async tests are for cases wh […]
You are correct, but, if we call setupElements() in onload and use test() as opposed to async_test() there is still no guarantee that the setupElements() method gets called before the tests. Async test here makes it possible so I can make sure
1- We do have a body,
2- Tests run after set up.
File third_party/WebKit/Source/core/dom/Element.cpp:
Patch Set #44, Line 620:
!RootScrollerUtil::IsGlobal(this) &&Thanks for explaining. […]
Thanks. I will add a link to CL description once a chromium.org version is ready.
File third_party/WebKit/Source/core/input/ScrollManager.cpp:
Patch Set #50, Line 754:
ScrollCustomization::ScrollDirection direction =
I can't see why it would be, what does the error say?
The error is not quite informative: "uses disallowed identifier". But it is resolved now (after using CamelCase).
File third_party/WebKit/Source/platform/scroll/ScrollCustomization.h:
Patch Set #50, Line 11:
blink {hmm... […]
Thanks for the hint. CamelCase worked.
Patch Set #50, Line 12:
namespace ScrollCustomization {IIRC style guide explicitly forbids relying on non uintX_t types if you need a specific width. […]
Yes I needed a header but was not sure what to use here. Based on your comment I went for wtf/ one.
These should be constexpr. […]
Yes. Makes sense. Thanks. That being said, wouldn't kScrollDirectionNone have to be just const though given that it is '0'?
File third_party/WebKit/Source/platform/scroll/ScrollCustomization.cpp:
Nit: Remove
Thanks. Done.
To view, visit change 590497. To unsubscribe, or for help writing mail filters, visit settings.
Timothy Dresser (Gerrit)
1 comment:
It seems unreasonable to require each scroll to take at least 50ms and that won't scale. […]
I agree with Navid that we shouldn't add a flag here. We should come up with some way of preventing fling that works in all browsers.
Bad idea: what about relying on the fact that key presses stop fling?
We could just hit the Ctrl key directly after the finger lifts?
Does that work in all browsers?
David Bokan (Gerrit)
3 comments:
I agree with Navid that we shouldn't add a flag here. […]
That could be flaky (maybe not in this case) since where the scroll stops will be time dependent. In general it would also add potentially unwanted side effects.
I'm not attached to any particular solution but I do think this will come up again at some point. I've noticed WebKit folks are already uneasy about how long WPT take to run.
Patch Set #48, Line 72:
assert_equals(elements[i].creationOrder, elements[i].calledOrder);You are correct, but, if we call setupElements() in onload and use test() as opposed to async_test() […]
When you call `test(myFunc, "Test myFunc)`, myFunc is run in the test() call. So:
setupElements();
test(...)Should guarantee setupElements is called before your test is run.
File third_party/WebKit/Source/platform/scroll/ScrollCustomization.h:
Yes. Makes sense. Thanks. […]
constexpr just means the value is available at compile time so:
constexpr ScrollDirection kScrollDirectionNone = 0;
Should work just fine.
Ehsan Karamad (Gerrit)
Thanks. PTAL.
3 comments:
Ack
So in conclusion, adding a <p> (or anything) before <script> implicitly created the <body> and now the test runs and passes just fine. Thanks.
Patch Set #48, Line 72:
window.internals.setScrollChain(scrollState, elements);When you call `test(myFunc, "Test myFunc)`, myFunc is run in the test() call. So: […]
Yes. So one test would have the setup element then. Thanks.
File third_party/WebKit/Source/platform/scroll/ScrollCustomization.h:
constexpr just means the value is available at compile time so: […]
Ack
To view, visit change 590497. To unsubscribe, or for help writing mail filters, visit settings.
David Bokan (Gerrit)
Some nits but % those, lgtm!
Patch set 53:Code-Review +1
2 comments:
Patch Set #53, Line 65:
setupElements();Move this outside the test()
File third_party/WebKit/Source/platform/scroll/ScrollCustomization.h:
Patch Set #53, Line 9:
platform/wtf/dtoa/utils.hIf it works in the bots, I'd actually use stdint.h. I suspect the extra defines in this header might be historical and we use stdint.h in other places. As is, it's not immediately clear why we'd be including double conversion utils in this file. Sorry for the churn.
David Bokan (Gerrit)
Also, my lgtm does not include anything in core/css. Please have a domain expert review those bits.
Ehsan Karamad (Gerrit)
Adding nainar@ for core/css reviews.
PTAL.
Timothy Dresser (Gerrit)
1 comment:
That could be flaky (maybe not in this case) since where the scroll stops will be time dependent. […]
I can imagine a solution of:
Fling.
Key Press.
Reset scroll position.Which should be non-flaky as long as long as we pick the right key to press.
I thought that setting scrollTop cancelled a fling, but that doesn't appear to be the case at this point.
Ehsan Karamad (Gerrit)
Ehsan Karamad uploaded patch set #54 to this change.
Introduce ScrollCustomization Property
This CL introduces the CSS property, 'scroll-customization'. Through
modifying scroll-customization, unnecessary calls to apply/distributeScroll
handlers can be avoided.
This CL implements the logic inside blink's main thread. If an scrollable
node has customized scrolling handlers, it can further specify which class
of gestures it is interested in for custom scrolling.
The list of valid values for customization includes 'auto', 'pan-x',
'pan-left', 'pan-right', 'pan-y', 'pan-up', and 'pan-down', and 'none'.
The default value is 'none' (any node interested in customized scroll
handlers should specify that).
For RootScroller we ignore customized scroll as it will regress certain
features which depend on global RootScroller.
This CL will eventually be followed up by changes to the compositor layer
so that for nodes with custom scroll, the scroll events corresponding
not allowed through scroll customization do not get sent to the main thread.
Link to document:
https://docs.google.com/document/d/1fB524MG5SJpajou7QdulqklciCLWTcoGa9olkPzVQVo/edit#heading=h.2b398wn8pdq
Bug: 765326
Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.android:android_optional_gpu_tests_rel
Change-Id: Ia54971b5535cb0b841682d242ef90761975f8c81
---
M third_party/WebKit/LayoutTests/LeakExpectations
M third_party/WebKit/LayoutTests/SlowTests
M third_party/WebKit/LayoutTests/fast/scroll-behavior/scroll-customization/scrollstate-distribute-to-scroll-chain-descendant.html
M third_party/WebKit/LayoutTests/fast/scroll-behavior/scroll-customization/touch-scroll-customization.html
A third_party/WebKit/LayoutTests/virtual/scroll_customization/fast/scroll-behavior/scroll-customization/scroll-customization-property.html
M third_party/WebKit/Source/core/BUILD.gn
M third_party/WebKit/Source/core/css/BUILD.gn
M third_party/WebKit/Source/core/css/CSSComputedStyleDeclaration.cpp
M third_party/WebKit/Source/core/css/CSSPrimitiveValueMappings.h
M third_party/WebKit/Source/core/css/CSSProperties.json5
M third_party/WebKit/Source/core/css/CSSValueKeywords.json5
M third_party/WebKit/Source/core/css/parser/CSSPropertyParserTest.cpp
M third_party/WebKit/Source/core/css/properties/ComputedStyleUtils.cpp
M third_party/WebKit/Source/core/css/properties/ComputedStyleUtils.h
A third_party/WebKit/Source/core/css/properties/longhands/ScrollCustomizationCustom.cpp
M third_party/WebKit/Source/core/dom/Element.cpp
M third_party/WebKit/Source/core/dom/Element.h
M third_party/WebKit/Source/core/frame/UseCounter.cpp
M third_party/WebKit/Source/core/input/ScrollManager.cpp
M third_party/WebKit/Source/core/input/ScrollManager.h
M third_party/WebKit/Source/core/page/scrolling/RootScrollerUtil.cpp
M third_party/WebKit/Source/core/page/scrolling/RootScrollerUtil.h
M third_party/WebKit/Source/core/page/scrolling/ScrollCustomizationCallbacks.cpp
M third_party/WebKit/Source/core/page/scrolling/ScrollCustomizationCallbacks.h
M third_party/WebKit/Source/core/page/scrolling/ScrollState.h
M third_party/WebKit/Source/devtools/front_end/sdk/CSSMetadata.js
M third_party/WebKit/Source/platform/BUILD.gn
A third_party/WebKit/Source/platform/scroll/ScrollCustomization.cpp
A third_party/WebKit/Source/platform/scroll/ScrollCustomization.h
M tools/metrics/histograms/enums.xml
30 files changed, 679 insertions(+), 43 deletions(-)
To view, visit change 590497. To unsubscribe, or for help writing mail filters, visit settings.
nainar (Gerrit)
core/css lgtm. You also have OWNERS lgtm on core/css thanks to bokan@'s review. :)
Patch set 54:Code-Review +1
Ehsan Karamad (Gerrit)
Thanks for the reviews.
I would need vollick@'s owner LGTM for platform/.
Adding pfeldman@ for CSSMetaData.js ownership review.
PTAL.
2 comments:
Patch Set #53, Line 65:
st(() => {
Move this outside the test()
Done. And also moved the logic inside test(()=> {}) like it used to be before. Thanks.
File third_party/WebKit/Source/platform/scroll/ScrollCustomization.h:
If it works in the bots, I'd actually use stdint.h. […]
Worked on Linux. Will see how other platforms fare against Linux.
To view, visit change 590497. To unsubscribe, or for help writing mail filters, visit settings.
Pavel Feldman (Gerrit)
Pavel Feldman (Gerrit)
Patch set 56:Code-Review +1
Ian Vollick (Gerrit)
platform/ lgtm
Ehsan Karamad (Gerrit)
Patch set 57:Commit-Queue +2
Commit Bot (Gerrit)
CQ is trying the patch.
Note: The patchset sent to CQ was uploaded after this CL was approved.
"Disabled the test on Mac" https://chromium-review.googlesource.com/c/590497/57
Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/chromium-review.googlesource.com/590497/57
Bot data: {"action": "start", "triggered_at": "2018-01-22T18:32:48.0Z", "cq_cfg_revision": "a668b5363cd374a29ca0f46124c226e2e2aa21d9", "revision": "d52e34aa0ba51fc08887189c7f3dace5a439fbd0"}
Commit Bot (Gerrit)
Commit Bot merged this change.
Introduce ScrollCustomization Property
This CL introduces the CSS property, 'scroll-customization'. Through
modifying scroll-customization, unnecessary calls to apply/distributeScroll
handlers can be avoided.
This CL implements the logic inside blink's main thread. If an scrollable
node has customized scrolling handlers, it can further specify which class
of gestures it is interested in for custom scrolling.
The list of valid values for customization includes 'auto', 'pan-x',
'pan-left', 'pan-right', 'pan-y', 'pan-up', and 'pan-down', and 'none'.
The default value is 'none' (any node interested in customized scroll
handlers should specify that).
For RootScroller we ignore customized scroll as it will regress certain
features which depend on global RootScroller.
This CL will eventually be followed up by changes to the compositor layer
so that for nodes with custom scroll, the scroll events corresponding
not allowed through scroll customization do not get sent to the main thread.
Link to document:
https://docs.google.com/document/d/1fB524MG5SJpajou7QdulqklciCLWTcoGa9olkPzVQVo/edit#heading=h.2b398wn8pdq
Bug: 765326
Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.android:android_optional_gpu_tests_rel
Change-Id: Ia54971b5535cb0b841682d242ef90761975f8c81
Reviewed-on: https://chromium-review.googlesource.com/590497
Commit-Queue: Ehsan Karamad <ekar...@chromium.org>
Reviewed-by: Pavel Feldman <pfel...@chromium.org>
Reviewed-by: Ian Vollick <vol...@chromium.org>
Reviewed-by: nainar <nai...@chromium.org>
Reviewed-by: David Bokan <bo...@chromium.org>
Reviewed-by: Ehsan Karamad <ekar...@chromium.org>
Cr-Commit-Position: refs/heads/master@{#530921}
---
M third_party/WebKit/LayoutTests/LeakExpectations
M third_party/WebKit/LayoutTests/SlowTests
M third_party/WebKit/LayoutTests/TestExpectations
31 files changed, 675 insertions(+), 43 deletions(-)