Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1895293002:
Test fragment scrolling and history restoration interaction w/ scroll anchoring. (Closed)
DescriptionTest fragment scrolling and history restoration interaction w/ scroll anchoring.
This CL allows RuntimeEnabledFeatures with the option 'set_from_internals' to be set
from Internals. Previously, we could only read whether a feature was enabled.
The tests in this CL verify that we do scroll anchoring when a page has a fragment
and when doing history restoration.
Note that there are no behavioral changes in this CL.
BUG=594880, 594879, 537764
Committed: https://crrev.com/5114bd75de8b0e7813df98a6a2c9b43943cd4229
Cr-Commit-Position: refs/heads/master@{#390463}
Patch Set 1 #
Total comments: 6
Patch Set 2 : extend internas api to set runtime enabled features + use testharness.js #
Total comments: 3
Patch Set 3 : Remove resetToConsistentState and conditionally make feature settable #
Total comments: 6
Patch Set 4 : fix broken layout tests #Patch Set 5 : address review comments #
Total comments: 6
Patch Set 6 : Remove .cpp.tmpl and other review comments #
Messages
Total messages: 42 (13 generated)
ymalik@chromium.org changed reviewers: + ojan@chromium.org
Spoke to you offline about this. We already do scroll anchoring for fragment scrolling and history restoration. I added some basic tests for this. You mentioned that scroll anchoring wasn't working correctly with history restoration, but that seems like another bug with picking an anchor node. AFAICT, we don't have to do anything special to handle these two cases and any bug here is probably a bug with the algorithm in general.
skobes has previously done C++ tests, which conveniently avoids adding internals APIs. See ScrollAnchorTest.cpp. The other alternative is to create a virtual test suite. An advantage of the latter is that other browsers would be able to use our tests as well. Although, in that case, you should use testharness.js, which is the W3C test harness library. I don't feel strongly between those two options. They both have tradeoffs. :) https://codereview.chromium.org/1895293002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/fast/layout/scroll-anchoring/fragment-scrolling-anchors.html (right): https://codereview.chromium.org/1895293002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/fast/layout/scroll-anchoring/fragment-scrolling-anchors.html:4: margin: 0px; height: 2000px; width: 2000px; Not a big deal, but the de facto standard for tests is to put each css property on its own line for easier readability. https://codereview.chromium.org/1895293002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/fast/layout/scroll-anchoring/fragment-scrolling-anchors.html:26: description("This test ensures that scroll anchoring interacts correctly with\ We don't usually have a line length limit on layout tests. https://codereview.chromium.org/1895293002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/fast/layout/scroll-anchoring/history-restore-anchors.html (right): https://codereview.chromium.org/1895293002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/fast/layout/scroll-anchoring/history-restore-anchors.html:56: window.location = "data:text/html,<script>history.back();</scr" + "ipt>"; I think you can just do window.location.reload(). That should also do scroll restoration the same way?
On 2016/04/19 21:34:24, ojan wrote: > skobes has previously done C++ tests, which conveniently avoids adding internals > APIs. See ScrollAnchorTest.cpp. The other alternative is to create a virtual > test suite. An advantage of the latter is that other browsers would be able to > use our tests as well. Although, in that case, you should use testharness.js, > which is the W3C test harness library. > > I don't feel strongly between those two options. They both have tradeoffs. :) > I think having a LayoutTest is more appropriate here because it's a somewhat closer representation of what's happening in the wild. The way I see the ScrollAnchorTest is that it is a good way to test the core algorithm, but ScrollAnchroing's interaction with things like history restoration and fragment scrolling are better as end-to-end LayoutTests. I agree the the plumbing into internals is non-ideal, but this will go away very soon when we promote ScrollAnchor into test (where the RuntimeEnabledFeature will be on by default). WDYT? I agree that using testharness.js is a better option so that other browsers can also use our tests. I was considering a virtual test suite, but it seems like they are meant to run a certain set of tests under additional options. If scroll anchoring becomes the default behavior, it makes less sense to have a virtual dir and we may want to move everything into the non-virtual directory? Is it okay keep this as a regular layout test (as it is currently) and use testharness.js instead of js-test.js?
On 2016/04/19 at 23:07:02, ymalik wrote: > On 2016/04/19 21:34:24, ojan wrote: > > skobes has previously done C++ tests, which conveniently avoids adding internals > > APIs. See ScrollAnchorTest.cpp. The other alternative is to create a virtual > > test suite. An advantage of the latter is that other browsers would be able to > > use our tests as well. Although, in that case, you should use testharness.js, > > which is the W3C test harness library. > > > > I don't feel strongly between those two options. They both have tradeoffs. :) > > > > I think having a LayoutTest is more appropriate here because it's a somewhat closer representation of what's happening in the wild. The way I see the ScrollAnchorTest is that it is a good way to test the core algorithm, but ScrollAnchroing's interaction with things like history restoration and fragment scrolling are better as end-to-end LayoutTests. I agree the the plumbing into internals is non-ideal, but this will go away very soon when we promote ScrollAnchor into test (where the RuntimeEnabledFeature will be on by default). WDYT? > > I agree that using testharness.js is a better option so that other browsers can also use our tests. > > I was considering a virtual test suite, but it seems like they are meant to run a certain set of tests under additional options. If scroll anchoring becomes the default behavior, it makes less sense to have a virtual dir and we may want to move everything into the non-virtual directory? Is it okay keep this as a regular layout test (as it is currently) and use testharness.js instead of js-test.js? Actually, thinking more about this, switching the flag to be a RuntimeEnabledFeature isn't blocked on getting all the existing tests to pass. If you leave out the status value, then the feature is disabled (even for tests). Then you can use the standard APIs for enabling RuntimeEnabledFeatures from tests. Unfortunately, noone has done the work to make runtime enabled features settable from internals. Right now it's read only. All we'd need to do for that is to change the script that autogenerated InternalRuntimeFlags.idl to include the setters as well. So, I think there are two approaches that you could take: 1. Make these tests a virtual test suite for now. We can undo that once we enable the feature by default. 2. Change the flag to be a RuntimeEnabledFeature and extend the internals APIs to allow setting arbirtrary RuntimeEnabledFeatures.
Description was changed from ========== Test fragment scrolling and history restoration interaction w/ scroll anchoring. This CL adds a temporary hook into InternalSettings to enable scroll anchoring from LayoutTests. This should be removed when crbug.com/597479 is resolved. The tests in this CL verify that we do scroll anchoring when a page has a fragment and when doing history restoration. Note that there are no behavioral changes in this CL. BUG=594880,594879 ========== to ========== Test fragment scrolling and history restoration interaction w/ scroll anchoring. This CL allows InternalRuntimeFlags to be set from Internals. Previously, we could only read whether a feature was enabled. The tests in this CL verify that we do scroll anchoring when a page has a fragment and when doing history restoration. Note that there are no behavioral changes in this CL. BUG=594880,594879 ==========
On 2016/04/22 01:04:14, ojan wrote: > On 2016/04/19 at 23:07:02, ymalik wrote: > > On 2016/04/19 21:34:24, ojan wrote: > > > skobes has previously done C++ tests, which conveniently avoids adding > internals > > > APIs. See ScrollAnchorTest.cpp. The other alternative is to create a virtual > > > test suite. An advantage of the latter is that other browsers would be able > to > > > use our tests as well. Although, in that case, you should use > testharness.js, > > > which is the W3C test harness library. > > > > > > I don't feel strongly between those two options. They both have tradeoffs. > :) > > > > > > > I think having a LayoutTest is more appropriate here because it's a somewhat > closer representation of what's happening in the wild. The way I see the > ScrollAnchorTest is that it is a good way to test the core algorithm, but > ScrollAnchroing's interaction with things like history restoration and fragment > scrolling are better as end-to-end LayoutTests. I agree the the plumbing into > internals is non-ideal, but this will go away very soon when we promote > ScrollAnchor into test (where the RuntimeEnabledFeature will be on by default). > WDYT? > > > > I agree that using testharness.js is a better option so that other browsers > can also use our tests. > > > > I was considering a virtual test suite, but it seems like they are meant to > run a certain set of tests under additional options. If scroll anchoring becomes > the default behavior, it makes less sense to have a virtual dir and we may want > to move everything into the non-virtual directory? Is it okay keep this as a > regular layout test (as it is currently) and use testharness.js instead of > js-test.js? > > Actually, thinking more about this, switching the flag to be a > RuntimeEnabledFeature isn't blocked on getting all the existing tests to pass. > If you leave out the status value, then the feature is disabled (even for > tests). Then you can use the standard APIs for enabling RuntimeEnabledFeatures > from tests. Unfortunately, noone has done the work to make runtime enabled > features settable from internals. Right now it's read only. All we'd need to do > for that is to change the script that autogenerated InternalRuntimeFlags.idl to > include the setters as well. > > So, I think there are two approaches that you could take: > 1. Make these tests a virtual test suite for now. We can undo that once we > enable the feature by default. > 2. Change the flag to be a RuntimeEnabledFeature and extend the internals APIs > to allow setting arbirtrary RuntimeEnabledFeatures. I ended up doing 2. because I figured it can be useful in the future. PTAL :)
PTAL :) Extend the internals APIs to allow setting RuntimeEnabledFeatures and use testharness.js in tests instead of js-test.js https://codereview.chromium.org/1895293002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/fast/layout/scroll-anchoring/fragment-scrolling-anchors.html (right): https://codereview.chromium.org/1895293002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/fast/layout/scroll-anchoring/fragment-scrolling-anchors.html:4: margin: 0px; height: 2000px; width: 2000px; On 2016/04/19 21:34:24, ojan wrote: > Not a big deal, but the de facto standard for tests is to put each css property > on its own line for easier readability. Done. https://codereview.chromium.org/1895293002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/fast/layout/scroll-anchoring/fragment-scrolling-anchors.html:26: description("This test ensures that scroll anchoring interacts correctly with\ On 2016/04/19 21:34:24, ojan wrote: > We don't usually have a line length limit on layout tests. This is removed in the new test. https://codereview.chromium.org/1895293002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/fast/layout/scroll-anchoring/history-restore-anchors.html (right): https://codereview.chromium.org/1895293002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/fast/layout/scroll-anchoring/history-restore-anchors.html:56: window.location = "data:text/html,<script>history.back();</scr" + "ipt>"; On 2016/04/19 21:34:24, ojan wrote: > I think you can just do window.location.reload(). That should also do scroll > restoration the same way? Done.
https://codereview.chromium.org/1895293002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/build/scripts/templates/InternalRuntimeFlags.cpp.tmpl (right): https://codereview.chromium.org/1895293002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/build/scripts/templates/InternalRuntimeFlags.cpp.tmpl:21: void InternalRuntimeFlags::resetToConsistentState() This doesn't appear to get called anywhere.
https://codereview.chromium.org/1895293002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/build/scripts/templates/InternalRuntimeFlags.cpp.tmpl (right): https://codereview.chromium.org/1895293002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/build/scripts/templates/InternalRuntimeFlags.cpp.tmpl:21: void InternalRuntimeFlags::resetToConsistentState() On 2016/04/22 20:48:31, ojan wrote: > This doesn't appear to get called anywhere. There was a comment in the header template that said that we should respect Internals::resetToConsistentState https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... I guess we should call it from there? I have a method in the idl file to call this to reset directly.
ymalik@chromium.org changed reviewers: + wkorman@chromium.org
@ojan, PTAL I removed resetToConsistetState from the the script that generates InternalRuntimeFlags. I also added a new option to RuntimeEnabledFeatures.in to make a feature conditionally settable from Internals (since it doesn't apply to all features). https://codereview.chromium.org/1895293002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/build/scripts/templates/InternalRuntimeFlags.cpp.tmpl (right): https://codereview.chromium.org/1895293002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/build/scripts/templates/InternalRuntimeFlags.cpp.tmpl:21: void InternalRuntimeFlags::resetToConsistentState() On 2016/04/22 20:57:19, ymalik1 wrote: > On 2016/04/22 20:48:31, ojan wrote: > > This doesn't appear to get called anywhere. > > There was a comment in the header template that said that we should respect > Internals::resetToConsistentState > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... > > I guess we should call it from there? I have a method in the idl file to call > this to reset directly. Spoke to you offline. No need to call resetToConsistentState as the flags are resetted in Internals::resetToConsistentState.
Description was changed from ========== Test fragment scrolling and history restoration interaction w/ scroll anchoring. This CL allows InternalRuntimeFlags to be set from Internals. Previously, we could only read whether a feature was enabled. The tests in this CL verify that we do scroll anchoring when a page has a fragment and when doing history restoration. Note that there are no behavioral changes in this CL. BUG=594880,594879 ========== to ========== Test fragment scrolling and history restoration interaction w/ scroll anchoring. This CL allows RuntimeEnabledFeatures with the option 'set_from_internals' to be set from Internals. Previously, we could only read whether a feature was enabled. The tests in this CL verify that we do scroll anchoring when a page has a fragment and when doing history restoration. Note that there are no behavioral changes in this CL. BUG=594880,594879,537764 ==========
On 2016/04/26 16:03:21, ymalik1 wrote: > @ojan, PTAL > > I removed resetToConsistetState from the the script that generates > InternalRuntimeFlags. > > I also added a new option to RuntimeEnabledFeatures.in to make a feature > conditionally settable from Internals (since it doesn't apply to all features). > > https://codereview.chromium.org/1895293002/diff/20001/third_party/WebKit/Sour... > File > third_party/WebKit/Source/build/scripts/templates/InternalRuntimeFlags.cpp.tmpl > (right): > > https://codereview.chromium.org/1895293002/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/build/scripts/templates/InternalRuntimeFlags.cpp.tmpl:21: > void InternalRuntimeFlags::resetToConsistentState() > On 2016/04/22 20:57:19, ymalik1 wrote: > > On 2016/04/22 20:48:31, ojan wrote: > > > This doesn't appear to get called anywhere. > > > > There was a comment in the header template that said that we should respect > > Internals::resetToConsistentState > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... > > > > I guess we should call it from there? I have a method in the idl file to call > > this to reset directly. > > Spoke to you offline. No need to call resetToConsistentState as the flags are > resetted in Internals::resetToConsistentState. Also note that fragment-scrolling-anchors.html passed without scroll anchoring. This is because we had the same anchoring behavior with fragment anchoring since the Webkit days. Though I couldn't find any test for it, so it's probably worth it to keep the test. Especially because we plan to upload this to web platform test.
https://codereview.chromium.org/1895293002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/build/scripts/templates/InternalRuntimeFlags.cpp.tmpl (right): https://codereview.chromium.org/1895293002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/build/scripts/templates/InternalRuntimeFlags.cpp.tmpl:14: : m_{{standard_features[0].first_lowered_name}}(RuntimeEnabledFeatures::{{standard_features[0].first_lowered_name}}Enabled()) Might be better to do this as a straight for-loop and conditionally pre-pend a comma. But I realize pragmatically we always have at least one. Would make code more concise, but generated code less readable. Thoughts? https://codereview.chromium.org/1895293002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/build/scripts/templates/InternalRuntimeFlags.h.tmpl (right): https://codereview.chromium.org/1895293002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/build/scripts/templates/InternalRuntimeFlags.h.tmpl:22: {% if feature.set_from_internals %} Rather than looping and checking feature.set_from_internals everywhere, we could generate the subset of settable features in a separate list like 'settable_from_internals' in make_internal_runtime_flags.py and then just iterate over that in all places needed. Could be easier to maintain? https://codereview.chromium.org/1895293002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in (right): https://codereview.chromium.org/1895293002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in:25: // set_from_internals specifies whether or not a feture can be set from core/testing/Internals.h/cpp, with the default whether a feature (remove 'or not', fix typo in 'feture') Since in the end people will be using internals.runtimeFlags in JS to set it maybe referencing that rather than the path to Internals.h/cpp is more useful?
@wkoman PTAL :) https://codereview.chromium.org/1895293002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/build/scripts/templates/InternalRuntimeFlags.cpp.tmpl (right): https://codereview.chromium.org/1895293002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/build/scripts/templates/InternalRuntimeFlags.cpp.tmpl:14: : m_{{standard_features[0].first_lowered_name}}(RuntimeEnabledFeatures::{{standard_features[0].first_lowered_name}}Enabled()) On 2016/04/26 19:01:18, wkorman wrote: > Might be better to do this as a straight for-loop and conditionally pre-pend a > comma. But I realize pragmatically we always have at least one. Would make code > more concise, but generated code less readable. Thoughts? Yeah. We're also doing it that way in the template for RuntimeEnabledFeatures, so I guess it would also be more consistent. https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... https://codereview.chromium.org/1895293002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/build/scripts/templates/InternalRuntimeFlags.h.tmpl (right): https://codereview.chromium.org/1895293002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/build/scripts/templates/InternalRuntimeFlags.h.tmpl:22: {% if feature.set_from_internals %} On 2016/04/26 19:01:18, wkorman wrote: > Rather than looping and checking feature.set_from_internals everywhere, we could > generate the subset of settable features in a separate list like > 'settable_from_internals' in make_internal_runtime_flags.py and then just > iterate over that in all places needed. Could be easier to maintain? I think that would only be the case if we plan to add additional logic that would conditionally set settable_from_internals on top of the value set in RuntimeEnabledFeatures.in. I think otherwise, its probably okay to just check for the option? I put it in one line to make it cleaner. It also makes it less cleaner to conditionally set 'readonly' from InternalRuntimeFlags.idl because we would have to do something like, if standard_feature and not in settabble_from_internals. I don't feel strong either way. WDYT? https://codereview.chromium.org/1895293002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in (right): https://codereview.chromium.org/1895293002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in:25: // set_from_internals specifies whether or not a feture can be set from core/testing/Internals.h/cpp, with the default On 2016/04/26 19:01:18, wkorman wrote: > whether a feature (remove 'or not', fix typo in 'feture') > > Since in the end people will be using internals.runtimeFlags in JS to set it > maybe referencing that rather than the path to Internals.h/cpp is more useful? Ah thanks! Done.
@wkoman PTAL :)
lgtm Thanks! Note I don't have OWNERS so unless you have it you'll still need from ojan@ or someone as well.
Sorry...one last thing. Why do we need to whitelist the things that are settable? Can we just make everything settable?
On 2016/04/26 23:24:07, ojan wrote: > Sorry...one last thing. Why do we need to whitelist the things that are > settable? Can we just make everything settable? In the comment (now deleted as part of this change) in InternalRuntimeFlags.h.tmpl there's a note that many/most flags won't actually work properly unless they are set from the start of the app via command-line or otherwise. It made sense to me to, as comment implies, make setters opt-in as a result. So eng basically only generates a setter when their feature will actually work with it.
On 2016/04/26 at 23:35:43, wkorman wrote: > On 2016/04/26 23:24:07, ojan wrote: > > Sorry...one last thing. Why do we need to whitelist the things that are > > settable? Can we just make everything settable? > > In the comment (now deleted as part of this change) in InternalRuntimeFlags.h.tmpl there's a note that many/most flags won't actually work properly unless they are set from the start of the app via command-line or otherwise. It made sense to me to, as comment implies, make setters opt-in as a result. So eng basically only generates a setter when their feature will actually work with it. I think it's pretty rare that a flag needs to be set from the command line, but that's fine. We can easily change this in the future to an opt-out.
The CQ bit was checked by ojan@chromium.org
lgtm
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1895293002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1895293002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
ymalik@chromium.org changed reviewers: + jbroman@chromium.org
+jbroman for Source/platform/RuntimeEnabledFeatures.in
https://codereview.chromium.org/1895293002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/build/scripts/templates/InternalRuntimeFlags.cpp.tmpl (right): https://codereview.chromium.org/1895293002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/build/scripts/templates/InternalRuntimeFlags.cpp.tmpl:4: #include "InternalRuntimeFlags.h" full include paths, please (assuming you keep this .cpp file): #include "core/testing/InternalRuntimeFlags.h" https://codereview.chromium.org/1895293002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/build/scripts/templates/InternalRuntimeFlags.h.tmpl (right): https://codereview.chromium.org/1895293002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/build/scripts/templates/InternalRuntimeFlags.h.tmpl:35: bool m_{{feature.first_lowered_name}}; Why do these members exist? You don't seem to actually use them; everything still seems to go to RuntimeEnabledFeatures. https://codereview.chromium.org/1895293002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/build/scripts/templates/InternalRuntimeFlags.idl.tmpl (right): https://codereview.chromium.org/1895293002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/build/scripts/templates/InternalRuntimeFlags.idl.tmpl:10: attribute boolean {{feature.first_lowered_name}}Enabled; Mind a comment somewhere (maybe here?) clarifying that these are indeed reset between layout tests? It took some digging for me to convince myself that this was safe, since the place where it's reset isn't obvious.
@jbroman PTAL :) https://codereview.chromium.org/1895293002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/build/scripts/templates/InternalRuntimeFlags.cpp.tmpl (right): https://codereview.chromium.org/1895293002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/build/scripts/templates/InternalRuntimeFlags.cpp.tmpl:4: #include "InternalRuntimeFlags.h" On 2016/04/28 14:24:39, jbroman wrote: > full include paths, please (assuming you keep this .cpp file): > > #include "core/testing/InternalRuntimeFlags.h" Removed .cpp. https://codereview.chromium.org/1895293002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/build/scripts/templates/InternalRuntimeFlags.h.tmpl (right): https://codereview.chromium.org/1895293002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/build/scripts/templates/InternalRuntimeFlags.h.tmpl:35: bool m_{{feature.first_lowered_name}}; On 2016/04/28 14:24:39, jbroman wrote: > Why do these members exist? You don't seem to actually use them; everything > still seems to go to RuntimeEnabledFeatures. You're right they're not needed. I had them here because in the previous patch I was using them to reset. Thanks! https://codereview.chromium.org/1895293002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/build/scripts/templates/InternalRuntimeFlags.idl.tmpl (right): https://codereview.chromium.org/1895293002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/build/scripts/templates/InternalRuntimeFlags.idl.tmpl:10: attribute boolean {{feature.first_lowered_name}}Enabled; On 2016/04/28 14:24:39, jbroman wrote: > Mind a comment somewhere (maybe here?) clarifying that these are indeed reset > between layout tests? It took some digging for me to convince myself that this > was safe, since the place where it's reset isn't obvious. Yes. Added to cpp.tmpl.
lgtm
The CQ bit was checked by ymalik@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from ojan@chromium.org, wkorman@chromium.org Link to the patchset: https://codereview.chromium.org/1895293002/#ps100001 (title: "Remove .cpp.tmpl and other review comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1895293002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1895293002/100001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by ymalik@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1895293002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1895293002/100001
Message was sent while issue was closed.
Description was changed from ========== Test fragment scrolling and history restoration interaction w/ scroll anchoring. This CL allows RuntimeEnabledFeatures with the option 'set_from_internals' to be set from Internals. Previously, we could only read whether a feature was enabled. The tests in this CL verify that we do scroll anchoring when a page has a fragment and when doing history restoration. Note that there are no behavioral changes in this CL. BUG=594880,594879,537764 ========== to ========== Test fragment scrolling and history restoration interaction w/ scroll anchoring. This CL allows RuntimeEnabledFeatures with the option 'set_from_internals' to be set from Internals. Previously, we could only read whether a feature was enabled. The tests in this CL verify that we do scroll anchoring when a page has a fragment and when doing history restoration. Note that there are no behavioral changes in this CL. BUG=594880,594879,537764 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/5114bd75de8b0e7813df98a6a2c9b43943cd4229 Cr-Commit-Position: refs/heads/master@{#390463}
Message was sent while issue was closed.
Description was changed from ========== Test fragment scrolling and history restoration interaction w/ scroll anchoring. This CL allows RuntimeEnabledFeatures with the option 'set_from_internals' to be set from Internals. Previously, we could only read whether a feature was enabled. The tests in this CL verify that we do scroll anchoring when a page has a fragment and when doing history restoration. Note that there are no behavioral changes in this CL. BUG=594880,594879,537764 ========== to ========== Test fragment scrolling and history restoration interaction w/ scroll anchoring. This CL allows RuntimeEnabledFeatures with the option 'set_from_internals' to be set from Internals. Previously, we could only read whether a feature was enabled. The tests in this CL verify that we do scroll anchoring when a page has a fragment and when doing history restoration. Note that there are no behavioral changes in this CL. BUG=594880,594879,537764 Committed: https://crrev.com/5114bd75de8b0e7813df98a6a2c9b43943cd4229 Cr-Commit-Position: refs/heads/master@{#390463} ========== |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
