Bug 64260 – [EFL] ewk_frame_hit_test_new enhancement

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit EFL

[Michal Pakula vel Rutka]

This patch enhances Ewk_Hit_Test structure by replacing structure flags with enum Ewk_Hit_Test_Result_Context and adding two fields containing image and media uri. Respective changes are also made in EWebLauncher.

[Michal Pakula vel Rutka]

Created attachment 100263 [details]
proposed patch

[Michal Pakula vel Rutka]

Created attachment 100284 [details]
fixed patch

[Michal Pakula vel Rutka]

Created attachment 100285 [details]
fixed patch

[Raphael Kubo da Costa (:rakuco)]

When you update the patch, please describe briefly what has changed in the new version, otherwise one needs to use "Diff" -> "Differences between" to find out.

> Source/WebKit/efl/ewk/ewk_frame.cpp:1068
> +    hit_test->context = (Ewk_Hit_Test_Result_Context)context;

Do you really need this cast? If so, please use a C++ cast.

> Tools/EWebLauncher/main.c:558
> +                   "%s"

Perhaps indent the lines here instead of below? This way you'd have

  "  %s\n"
  /* code */
  ht->context & EWK_HIT_TEST_RESULT_FOO ? "FOO" : "",

> Tools/EWebLauncher/main.c:573
> +                   ht->context & EWK_HIT_TEST_RESULT_CONTEXT_LINK ? "  LINK\n": "",

Pleas add a space before the ":"

[Michal Pakula vel Rutka]

(In reply to comment #4)
> When you update the patch, please describe briefly what has changed in the new version, otherwise one needs to use "Diff" -> "Differences between" to find out.
> 

OK

> > Source/WebKit/efl/ewk/ewk_frame.cpp:1068
> > +    hit_test->context = (Ewk_Hit_Test_Result_Context)context;
> 
> Do you really need this cast? If so, please use a C++ cast.
> 

Yes, I think is needed here, anyway I will change it to static_cast.

> > Tools/EWebLauncher/main.c:558
> > +                   "%s"
> 
> Perhaps indent the lines here instead of below? This way you'd have
> 
>   "  %s\n"
>   /* code */
>   ht->context & EWK_HIT_TEST_RESULT_FOO ? "FOO" : "",
> 

But then when ht->context & EWK_HIT_TEST_RESULT_FOO will be false we will get two spaces and there will be problem with aligning contexts.

> > Tools/EWebLauncher/main.c:573
> > +                   ht->context & EWK_HIT_TEST_RESULT_CONTEXT_LINK ? "  LINK\n": "",
> 
> Pleas add a space before the ":"

OK

[Raphael Kubo da Costa (:rakuco)]

(In reply to comment #5)
> > > Tools/EWebLauncher/main.c:558
> > > +                   "%s"
> > 
> > Perhaps indent the lines here instead of below? This way you'd have
> > 
> >   "  %s\n"
> >   /* code */
> >   ht->context & EWK_HIT_TEST_RESULT_FOO ? "FOO" : "",
> > 
> 
> But then when ht->context & EWK_HIT_TEST_RESULT_FOO will be false we will get two spaces and there will be problem with aligning contexts.

Oh, you're right. Sorry.

[Michal Pakula vel Rutka]

Created attachment 100296 [details]
changed C-cast to static_cast + code style according to Raphael's suggestions

[Raphael Kubo da Costa (:rakuco)]

LGTM.

[Gyuyoung Kim]

LGTM too.

[WebKit Review Bot]

Comment on attachment 100296 [details]
changed C-cast to static_cast + code style according to Raphael's suggestions

Clearing flags on attachment: 100296

Committed r91572: <http://trac.webkit.org/changeset/91572>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.