Bug 89095 - [EFL][WK2] Add title support to Ewk_View
Summary: [EFL][WK2] Add title support to Ewk_View
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords:
Depends on:
Blocks: 61838
  Show dependency treegraph
 
Reported: 2012-06-14 06:42 PDT by Chris Dumez
Modified: 2012-06-15 05:17 PDT (History)
5 users (show)

See Also:


Attachments
Patch (11.72 KB, patch)
2012-06-14 06:50 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (11.66 KB, patch)
2012-06-15 03:28 PDT, Chris Dumez
kenneth: review+
cdumez: commit-queue-
Details | Formatted Diff | Diff
Patch for landing (11.70 KB, patch)
2012-06-15 04:03 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-06-14 06:42:26 PDT
We need to add support for title getting and title change notification to Ewk_View.

This can then be used in MiniBrowser to set / update the window title.
Comment 1 Chris Dumez 2012-06-14 06:50:00 PDT
Created attachment 147575 [details]
Patch
Comment 2 Ryuan Choi 2012-06-14 16:17:53 PDT
Looks good to me.
Comment 3 Chris Dumez 2012-06-15 01:10:03 PDT
I need this patch also for Ewk integration of Web Intents because it has the WKPageLoaderClient. I will need to define a callback for dispatchIntent() and emit a signal with the Ewk_Intent.
Comment 4 Eunmi Lee 2012-06-15 01:12:35 PDT
> Source/WebKit2/UIProcess/API/efl/ewk_loader_client.cpp:29
> +#include "ewk_loader_client_private.h"
I think "ewk_loader_client_private.h"'s proper location is #27 line.

> Source/WebKit2/UIProcess/API/efl/ewk_loader_client.cpp:30
> +#include "ewk_view.h"
I think "ewk_view.h" is not needed in this file.

> Source/WebKit2/UIProcess/API/efl/ewk_loader_client.cpp:45
> +void ewk_loader_client_attach(WKPageRef pageRef, Evas_Object* ewkView)
How about changing the function name to "ewk_view_loader_client_attach(Evas_Object* ewkView, WKPageRef pageRef)" and file name to "ewk_view_loader_client.cpp"?
I think it it better to use "ewk_view" prefix because loader client will be set for ewk_view.

> Source/WebKit2/UIProcess/API/efl/ewk_loader_client_private.h: 32
> +void ewk_loader_client_attach(WKPageRef pageRef, Evas_Object *o);
Would you change "Evas_Object *o" to "Evas_Object* o"? This file has to follow WebKit style because it is private header.

> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:549
> +evas_object_smart_callback_call(ewkView, "title,changed", (void*)title);
Please use static_cast<void*>() instead of (void*).
Comment 5 Chris Dumez 2012-06-15 01:26:14 PDT
Comment on attachment 147575 [details]
Patch

Clear flags until I update the patch. Thanks for the feedback.
Comment 6 Chris Dumez 2012-06-15 01:31:36 PDT
(In reply to comment #4)
> > Source/WebKit2/UIProcess/API/efl/ewk_loader_client.cpp:29
> > +#include "ewk_loader_client_private.h"
> I think "ewk_loader_client_private.h"'s proper location is #27 line.

What you're suggesting would be right if it was a public header (i.e. "ewk_loader_client.h"). However, I believe the current location is correct considering it is a private header. Also, if I move the line, the style check script starts complaining.
Comment 7 Eunmi Lee 2012-06-15 03:26:21 PDT
(In reply to comment #6)
> (In reply to comment #4)
> > > Source/WebKit2/UIProcess/API/efl/ewk_loader_client.cpp:29
> > > +#include "ewk_loader_client_private.h"
> > I think "ewk_loader_client_private.h"'s proper location is #27 line.
> 
> What you're suggesting would be right if it was a public header (i.e. "ewk_loader_client.h"). However, I believe the current location is correct considering it is a private header. Also, if I move the line, the style check script starts complaining.

Oh, I see. you are right :)
Comment 8 Chris Dumez 2012-06-15 03:28:51 PDT
Created attachment 147783 [details]
Patch

Take feedback into consideration.
Comment 9 Chris Dumez 2012-06-15 04:00:51 PDT
Comment on attachment 147783 [details]
Patch

Needs rebasing due to other patch that just landed.
Comment 10 Chris Dumez 2012-06-15 04:03:49 PDT
Created attachment 147791 [details]
Patch for landing

Rebasing on master. Could someone please cq+ ?
Comment 11 WebKit Review Bot 2012-06-15 05:17:12 PDT
Comment on attachment 147791 [details]
Patch for landing

Clearing flags on attachment: 147791

Committed r120442: <http://trac.webkit.org/changeset/120442>
Comment 12 WebKit Review Bot 2012-06-15 05:17:17 PDT
All reviewed patches have been landed.  Closing bug.