Summary: | Fix typo in StyleResolveTree.cpp | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Maksim Kisilev <mkisilev> | ||||||||
Component: | WebCore Misc. | Assignee: | Brent Fulgham <bfulgham> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Minor | CC: | bfulgham, commit-queue, dbates, mkisilev | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | iPhone / iPad | ||||||||||
OS: | Unspecified | ||||||||||
Bug Depends on: | 120173 | ||||||||||
Bug Blocks: | |||||||||||
Attachments: |
|
Description
Maksim Kisilev
2014-12-25 09:41:31 PST
Created attachment 243745 [details]
Patch
Attachment 243745 [details] did not pass style-queue:
ERROR: Source/WebCore/ChangeLog:8: You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible. [changelog/nonewtests] [5]
Total errors found: 1 in 2 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 243746 [details]
Patch
Comment on attachment 243746 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=243746&action=review > Source/WebCore/ChangeLog:8 > + No new tests. Behaviour wasn't changed. If there is no change in behavior, what is the rationale for making this code change? (In reply to comment #4) > Comment on attachment 243746 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=243746&action=review > > > Source/WebCore/ChangeLog:8 > > + No new tests. Behaviour wasn't changed. > > If there is no change in behavior, what is the rationale for making this > code change? Final behavior wasn't changed. But code before change had meaningless check for: WKContentChange() != WKContentVisibilityChange (In reply to comment #5) > (In reply to comment #4) > > Comment on attachment 243746 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=243746&action=review > > > > > Source/WebCore/ChangeLog:8 > > > + No new tests. Behaviour wasn't changed. > > > > If there is no change in behavior, what is the rationale for making this > > code change? > > Final behavior wasn't changed. But code before change had meaningless check > for: > WKContentChange() != WKContentVisibilityChange Yes, this code is wrong. WKContentChange() is not a function, it's a type -- so this is like saying "int() != WKContentVisibilityChange" The correct method is WKObservedContentChange(). I'm not sure what impact this has in practice, but it should be corrected. Created attachment 273584 [details]
Patch
I rebaselined Maksim's patch. Let's see what EWS says... EWS was happy with this change. Comment on attachment 273584 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=273584&action=review > Source/WebCore/ChangeLog:8 > + No new tests. Behaviour wasn't changed. You state below that you actually expect some change in behavior, so I don't think you can claim nothing changed. I'm trying to see if I can figure out a test case for this. This code is only used on iOS, and only triggers in the mouseMoved and 'handleSyntheticClick" event handler. Businessinsider.com helpfully generates synthetic clicks for you, which shows the code getting hit. With the bug, we always use the "elementImplicitVisibility" path. This means that an element might stay hidden that (according to this code) should have been forced to be VISIBLE. (In reply to comment #11) > I'm trying to see if I can figure out a test case for this. > You may find inspiration from looking at the tests included in <http://trac.webkit.org/changeset/194038> (bug #144451). In particular, see test <http://trac.webkit.org/browser/trunk/LayoutTests/fast/events/can-click-element-on-page-with-active-pseudo-class-and-search-field.html>. We may have additional tests that exercise the CheckForVisibilityChangeOnRecalcStyle code in <http://trac.webkit.org/browser/trunk/LayoutTests/platform/ios-simulator/ios>. Committed r198044: <http://trac.webkit.org/changeset/198044> |