Summary: | Move allowRoundingHacks to Internals interface | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Gyuyoung Kim <gyuyoung.kim> | ||||||||||||
Component: | Tools / Tests | Assignee: | Gyuyoung Kim <gyuyoung.kim> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | ap, dglazkov, eric, jberlin, laszlo.gombos, morrita, rakuco, simon.fraser, webkit.review.bot | ||||||||||||
Priority: | P2 | ||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||
Hardware: | Unspecified | ||||||||||||||
OS: | Unspecified | ||||||||||||||
Attachments: |
|
Description
Gyuyoung Kim
2012-05-23 18:15:09 PDT
Created attachment 143692 [details]
Patch
CC'ing Alexey, could you take a look this patch when you have time ? Comment on attachment 143692 [details] Patch Attachment 143692 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/12791062 Created attachment 143715 [details]
Patch
Comment on attachment 143715 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=143715&action=review > Source/WebKit/mac/WebView/WebViewPrivate.h:-347 > -+ (void)_setAllowsRoundingHacks:(BOOL)allowsRoundingHacks; > -+ (BOOL)_allowsRoundingHacks; Do not remove this WebKit API. Created attachment 143723 [details]
Patch
(In reply to comment #5) > (From update of attachment 143715 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=143715&action=review > > > Source/WebKit/mac/WebView/WebViewPrivate.h:-347 > > -+ (void)_setAllowsRoundingHacks:(BOOL)allowsRoundingHacks; > > -+ (BOOL)_allowsRoundingHacks; > > Do not remove this WebKit API. Ok, I restore WebKit API for mac. Comment on attachment 143723 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=143723&action=review > Source/WebCore/testing/Internals.cpp:1057 > +void Internals::allowRoundingHacks() const > +{ > + TextRun::setAllowsRoundingHacks(true); > +} How does this get reset between test runs? Comment on attachment 143723 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=143723&action=review >> Source/WebCore/testing/Internals.cpp:1057 >> +} > > How does this get reset between test runs? Currently, this LTC function is implemented by Mac. As you can see in LayoutTestControllerMac.mm, LayoutTestController::allowRoundingHacks() just sets true. void LayoutTestController::allowRoundingHacks() { [WebView _setAllowsRoundingHacks:YES]; } It looks there is no reset function in LTC now. This patch is just to move from LTC function to Internals. Do you think I need to modify it together with this patch ? Yes, but it's reset between tests in DRT: http://trac.webkit.org/browser/trunk/Tools/DumpRenderTree/mac/DumpRenderTree.mm#L1271 I believe Internals has a similar concept, w/o doing such, we'll end up bleeding this setting between tests. (In reply to comment #10) > Yes, but it's reset between tests in DRT: > http://trac.webkit.org/browser/trunk/Tools/DumpRenderTree/mac/DumpRenderTree.mm#L1271 > > I believe Internals has a similar concept, w/o doing such, we'll end up bleeding this setting between tests. Ok, I will check if Internals has similar concept to reset settings. If internals has similar one, I think we can move more LTC functions to Internals. Created attachment 144266 [details]
Patch
Created attachment 144268 [details]
Patch
I add a function to Internals in order to reset default consistent values like each port's LTC. I think Internals also needs to have reset function. Comment on attachment 144268 [details]
Patch
I'm wondering a bit if this need to be cross platform since the tests are under platform/mac.
But it's fine to have this on Internals even if so.
(In reply to comment #15) > (From update of attachment 144268 [details]) > I'm wondering a bit if this need to be cross platform since the tests are under platform/mac. I expect other ports will support allowRoundingHacks() testing in future. So, in my humble opinion, to move allowRoundingHacks() to Internals is valid. In addition, Internals also needs to have reset functions for other test cases. Thank you for your review. Comment on attachment 144268 [details] Patch Clearing flags on attachment: 144268 Committed r118657: <http://trac.webkit.org/changeset/118657> All reviewed patches have been landed. Closing bug. allowRoundingHacks() is a terrible name for a method on internals, because: 1. It's not clear if it's querying state (areRoundingHacksAllowed()) or setting state (setRoundingHacksAllowed()). If the latter, why doesn't it take a bool? 2. I've no idea what rounding hacks are. The name should be more specific. (In reply to comment #19) > allowRoundingHacks() is a terrible name for a method on internals, because: > 1. It's not clear if it's querying state (areRoundingHacksAllowed()) or setting state (setRoundingHacksAllowed()). If the latter, why doesn't it take a bool? > 2. I've no idea what rounding hacks are. The name should be more specific. Simon, your comment looks reasonable. How about changing allowRoundingHacks() with setAllowRoundingHacks(bool) ? *** Bug 63477 has been marked as a duplicate of this bug. *** |