Bug 86475 – [EFL] EFL's LayoutTestController does not support titleTextDirection

• Component: WebKit EFL
• Status: RESOLVED FIXED

[Chris Dumez]

EFL's LayoutTestController does not support titleTextDirection which is needed by:
  fast/dom/title-directionality.html
  fast/dom/title-directionality-removeChild.html

[Chris Dumez]

Created attachment 144002 [details]
Patch

[Raphael Kubo da Costa (:rakuco)]

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?

[Chris Dumez]

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.

[Chris Dumez]

Created attachment 144322 [details]
Patch

[Raphael Kubo da Costa (:rakuco)]

Comment on attachment 144322 [details]
Patch

Looks good to me, thanks.

[Gyuyoung Kim]

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.

[Chris Dumez]

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

[Gyuyoung Kim]

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

[Raphael Kubo da Costa (:rakuco)]

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

[Gyuyoung Kim]

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

[Hajime Morrita]

Comment on attachment 144322 [details]
Patch

rs=me

[WebKit Review Bot]

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

[Chris Dumez]

Created attachment 147754 [details]
Patch

[WebKit Review Bot]

Comment on attachment 147754 [details]
Patch

Clearing flags on attachment: 147754

Committed r120428: <http://trac.webkit.org/changeset/120428>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.