Bug 64260 - [EFL] ewk_frame_hit_test_new enhancement
Summary: [EFL] ewk_frame_hit_test_new enhancement
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit EFL (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other Linux
: P4 Enhancement
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-07-11 04:28 PDT by Michal Pakula vel Rutka
Modified: 2011-07-22 10:31 PDT (History)
5 users (show)

See Also:


Attachments
proposed patch (6.29 KB, patch)
2011-07-11 04:29 PDT, Michal Pakula vel Rutka
no flags Details | Formatted Diff | Diff
fixed patch (6.19 KB, patch)
2011-07-11 06:04 PDT, Michal Pakula vel Rutka
no flags Details | Formatted Diff | Diff
fixed patch (6.19 KB, patch)
2011-07-11 06:13 PDT, Michal Pakula vel Rutka
no flags Details | Formatted Diff | Diff
changed C-cast to static_cast + code style according to Raphael's suggestions (6.21 KB, patch)
2011-07-11 07:14 PDT, Michal Pakula vel Rutka
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Michal Pakula vel Rutka 2011-07-11 04:28:17 PDT
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.
Comment 1 Michal Pakula vel Rutka 2011-07-11 04:29:48 PDT
Created attachment 100263 [details]
proposed patch
Comment 2 Michal Pakula vel Rutka 2011-07-11 06:04:08 PDT
Created attachment 100284 [details]
fixed patch
Comment 3 Michal Pakula vel Rutka 2011-07-11 06:13:10 PDT
Created attachment 100285 [details]
fixed patch
Comment 4 Raphael Kubo da Costa (:rakuco) 2011-07-11 06:32:14 PDT
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 ":"
Comment 5 Michal Pakula vel Rutka 2011-07-11 06:56:57 PDT
(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
Comment 6 Raphael Kubo da Costa (:rakuco) 2011-07-11 07:01:37 PDT
(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.
Comment 7 Michal Pakula vel Rutka 2011-07-11 07:14:35 PDT
Created attachment 100296 [details]
changed C-cast to static_cast + code style according to Raphael's suggestions
Comment 8 Raphael Kubo da Costa (:rakuco) 2011-07-11 08:41:09 PDT
LGTM.
Comment 9 Gyuyoung Kim 2011-07-11 17:48:18 PDT
LGTM too.
Comment 10 WebKit Review Bot 2011-07-22 10:31:17 PDT
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>
Comment 11 WebKit Review Bot 2011-07-22 10:31:22 PDT
All reviewed patches have been landed.  Closing bug.