EFL's LayoutTestController does not support titleTextDirection which is needed by: fast/dom/title-directionality.html fast/dom/title-directionality-removeChild.html
Created attachment 144002 [details] Patch
Comment on attachment 144002 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=144002&action=review > Source/WebKit/efl/ChangeLog:8 > + Fix setting of title direction in ewk_frame_title_set() and I didn't really understand what was wrong with the calls. Can you explain why you need to take an address to change the direction member? > Source/WebKit/efl/ewk/ewk_frame.cpp:-1582 > - if (!eina_stringshare_replace(&smartData->title.string, title->string)) > + bool directionChanged = false; > + if (smartData->title.direction != title->direction) { > + (&smartData->title)->direction = title->direction; > + directionChanged = true; > + } > + if (!eina_stringshare_replace(&smartData->title.string, title->string) && !directionChanged) > return; > - smartData->title.direction = title->direction; How about if ((!eina_stringshare_replace(...)) && (smartData->title.string == title->direction)) return; smartData->title.direction = ...; > Tools/DumpRenderTree/LayoutTestController.cpp:2172 > + if (!titleDirection.get()) > + return JSValueMakeUndefined(context); I wonder if this can really happen, as titleTextDirection should always return a valid std::string. Perhaps an ASSERT() is enough?
Comment on attachment 144002 [details] Patch You're right, the dereferencing for the structure is probably not needed. I'll double check and fix the patch.
Created attachment 144322 [details] Patch
Comment on attachment 144322 [details] Patch Looks good to me, thanks.
Comment on attachment 144322 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=144322&action=review > Tools/DumpRenderTree/LayoutTestController.cpp:2278 > + { "titleTextDirection", getTitleTextDirectionCallback, 0, kJSPropertyAttributeReadOnly | kJSPropertyAttributeDontDelete }, I wonder whether this value can be used by all ports. Because you just implement this static value for EFL port.
(In reply to comment #6) > (From update of attachment 144322 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=144322&action=review > > > Tools/DumpRenderTree/LayoutTestController.cpp:2278 > > + { "titleTextDirection", getTitleTextDirectionCallback, 0, kJSPropertyAttributeReadOnly | kJSPropertyAttributeDontDelete }, > > I wonder whether this value can be used by all ports. Because you just implement this static value for EFL port. I provide empty implementation for other ports so that it will compile. Then other ports will need to provide an implementation if they really want to support this LayoutTestController attribute. This is the usual way (at least for methods, I don't know if attributes are any different). I don't see any problem here. Do you expect an "#if PLATFORM(EFL)" ?
(In reply to comment #7) > (In reply to comment #6) > > (From update of attachment 144322 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=144322&action=review > > > > > Tools/DumpRenderTree/LayoutTestController.cpp:2278 > > > + { "titleTextDirection", getTitleTextDirectionCallback, 0, kJSPropertyAttributeReadOnly | kJSPropertyAttributeDontDelete }, > > > > I wonder whether this value can be used by all ports. Because you just implement this static value for EFL port. > > I provide empty implementation for other ports so that it will compile. Then other ports will need to provide an implementation if they really want to support this LayoutTestController attribute. This is the usual way (at least for methods, I don't know if attributes are any different). I don't see any problem here. Do you expect an "#if PLATFORM(EFL)" ? I think it would good to provide dummy implementation for other ports. Did you already support it ? I couldn't find it. If so, please let me know the bug.
(In reply to comment #8) > I think it would good to provide dummy implementation for other ports. Did you already support it ? I couldn't find it. If so, please let me know the bug. I don't understand this proposal: the implementation itself is in LayoutTestController itself, what each port needs to do is simply call LTC::setTitleTextDirection() when they need it.
(In reply to comment #9) > (In reply to comment #8) > > I think it would good to provide dummy implementation for other ports. Did you already support it ? I couldn't find it. If so, please let me know the bug. > > I don't understand this proposal: the implementation itself is in LayoutTestController itself, what each port needs to do is simply call LTC::setTitleTextDirection() when they need it. Hmm, it looks there was misunderstanding for me. If there is no need to implement on port side, this patch is good. I'm sorry.
Comment on attachment 144322 [details] Patch rs=me
Comment on attachment 144322 [details] Patch Rejecting attachment 144322 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2 Last 500 characters of output: tching file Tools/DumpRenderTree/LayoutTestController.h Hunk #1 succeeded at 359 (offset -1 lines). Hunk #2 succeeded at 425 (offset -1 lines). patching file Tools/DumpRenderTree/efl/DumpRenderTreeChrome.cpp Hunk #1 FAILED at 451. 1 out of 1 hunk FAILED -- saving rejects to file Tools/DumpRenderTree/efl/DumpRenderTreeChrome.cpp.rej Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force', u'--reviewer', u'Hajime Mor..." exit_code: 1 cwd: /mnt/git/webkit-commit-queue/ Full output: http://queues.webkit.org/results/12965388
Created attachment 147754 [details] Patch
Comment on attachment 147754 [details] Patch Clearing flags on attachment: 147754 Committed r120428: <http://trac.webkit.org/changeset/120428>
All reviewed patches have been landed. Closing bug.