[webkit-reviews] review requested: [Bug 54623] RTL web content should have left-hand scrollbar. : [Attachment 121427] Patch v8 (refactored)

[bugzilla-daemon at webkit.org]

Hironori Bono <hbono at chromium.org> has asked  for review:
Bug 54623: RTL web content should have left-hand scrollbar.
https://bugs.webkit.org/show_bug.cgi?id=54623

Attachment 121427: Patch v8 (refactored)
https://bugs.webkit.org/attachment.cgi?id=121427&action=review

------- Additional Comments from Hironori Bono <hbono at chromium.org>
Greetings,

Many thanks for your review and comments. Also sorry for my slow update.
This feature s enabled only on Chromium just because Dave told Safari did not
need it as written in his comment #12. (Even though I'm not sure if other ports
need it, I would like to disable it because it may surprise them to change the
behavior of scrollbars without noticing them.)
As you pointed out, my previous change has lots of copy-and-paste code. (This
is just because I could not find good function names.) I have created three new
functions (rtlAwareCornerX, rtlAwareVerticalScrollbarX, and
rtlAwareHorizontalScrollbarX) to simplify this change. (Unfortunately, we need
three functions: the first one is for moving a resizer, the second one is for
moving a vertical scrollbar, and the third one is for moving a horizontal
scrollbar, respectively.) Moving a vertical scrollbar needs to move a
horizontal scrollbar and a resizer.

Regards,

Hironori Bono

(In reply to comment #36)
> (From update of attachment 114018 [details])
> This seems like a lot of copy-paste code.
> 
> I'm willing to believe that this *is* the right solution, but I need you to
convince me of that.
> 
> Why is this the right abstraction layer?  Can we make this change at a lower
level?	Could we add helper functions like "adjustRectForRTLAwareScrollBar"
which would contain this ifdef and do nothing when this ifdef is disabled?
> 
> I'm nto sure what the right way to write this change is.  You are *much* more
familiar with this code than I am, and I trust your judgement!
> 
> I do, however need you to convince me, and future readers of this patch, (in
the ChangeLog, ideally) that this is the right way to go.
> 
> If you can come up with a solution which involves less copy-paste code, that
would be ideal.
> 
> Thanks again for the patch!