Bug 86475 - [EFL] EFL's LayoutTestController does not support titleTextDirection
Summary: [EFL] EFL's LayoutTestController does not support titleTextDirection
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit EFL (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords:
Depends on: 86462
Blocks:
  Show dependency treegraph
 
Reported: 2012-05-15 06:13 PDT by Chris Dumez
Modified: 2012-06-15 02:24 PDT (History)
7 users (show)

See Also:


Attachments
Patch (8.92 KB, patch)
2012-05-25 01:24 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (7.87 KB, patch)
2012-05-28 03:51 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (8.52 KB, patch)
2012-06-15 00:25 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Dumez 2012-05-15 06:13:37 PDT
EFL's LayoutTestController does not support titleTextDirection which is needed by:
  fast/dom/title-directionality.html
  fast/dom/title-directionality-removeChild.html
Comment 1 Chris Dumez 2012-05-25 01:24:37 PDT
Created attachment 144002 [details]
Patch
Comment 2 Raphael Kubo da Costa (:rakuco) 2012-05-25 15:03:28 PDT
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 3 Chris Dumez 2012-05-26 02:28:24 PDT
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.
Comment 4 Chris Dumez 2012-05-28 03:51:35 PDT
Created attachment 144322 [details]
Patch
Comment 5 Raphael Kubo da Costa (:rakuco) 2012-05-28 06:36:29 PDT
Comment on attachment 144322 [details]
Patch

Looks good to me, thanks.
Comment 6 Gyuyoung Kim 2012-05-29 04:10:55 PDT
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.
Comment 7 Chris Dumez 2012-05-29 04:22:33 PDT
(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)" ?
Comment 8 Gyuyoung Kim 2012-05-29 09:52:43 PDT
(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.
Comment 9 Raphael Kubo da Costa (:rakuco) 2012-05-29 10:23:29 PDT
(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.
Comment 10 Gyuyoung Kim 2012-05-29 18:13:43 PDT
(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 11 Hajime Morrita 2012-06-15 00:05:00 PDT
Comment on attachment 144322 [details]
Patch

rs=me
Comment 12 WebKit Review Bot 2012-06-15 00:09:15 PDT
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
Comment 13 Chris Dumez 2012-06-15 00:25:15 PDT
Created attachment 147754 [details]
Patch
Comment 14 WebKit Review Bot 2012-06-15 02:24:21 PDT
Comment on attachment 147754 [details]
Patch

Clearing flags on attachment: 147754

Committed r120428: <http://trac.webkit.org/changeset/120428>
Comment 15 WebKit Review Bot 2012-06-15 02:24:26 PDT
All reviewed patches have been landed.  Closing bug.