Bug 87328 – Move allowRoundingHacks to Internals interface

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Gyuyoung Kim]

Adjust allowRoundingHacks() tests into use Internals instead of LayoutTestController interface. In my humble opinion, allowRoundingHacks() is able to be supported by Internals because it looks this function is implemented by WebCore layer, not WebKit layer. It seems allowRoundingHacks() is just to turn TextRun::s_allowsRoundingHacks variable on.

If reviewers think this moving is not able to be moved to Internals, I'm willing to close this bug.

[Gyuyoung Kim]

Created attachment 143692 [details]
Patch

[Gyuyoung Kim]

CC'ing Alexey, could you take a look this patch when you have time ?

[Build Bot]

Comment on attachment 143692 [details]
Patch

Attachment 143692 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12791062

[Gyuyoung Kim]

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.

[Gyuyoung Kim]

Created attachment 143723 [details]
Patch

[Gyuyoung Kim]

(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.

[Eric Seidel (no email)]

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?

[Gyuyoung Kim]

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 ?

[Eric Seidel (no email)]

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.

[Gyuyoung Kim]

(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.

[Gyuyoung Kim]

Created attachment 144266 [details]
Patch

[Gyuyoung Kim]

Created attachment 144268 [details]
Patch

[Gyuyoung Kim]

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.

[Hajime Morrita]

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.

[Gyuyoung Kim]

(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.

[WebKit Review Bot]

Comment on attachment 144268 [details]
Patch

Clearing flags on attachment: 144268

Committed r118657: <http://trac.webkit.org/changeset/118657>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Simon Fraser (smfr)]

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.

[Gyuyoung Kim]

(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) ?

[Jessie Berlin]

*** Bug 63477 has been marked as a duplicate of this bug. ***