WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
68074
[GTK][WEBKIT2] Add support for title property in WebKitWebView
https://bugs.webkit.org/show_bug.cgi?id=68074
Summary
[GTK][WEBKIT2] Add support for title property in WebKitWebView
Nayan Kumar K
Reported
2011-09-14 04:20:58 PDT
Add support for title and progress property in WebKitWebView and modify GtkLauncher2 to handle these properties
Attachments
Add title and progress properties
(13.20 KB, patch)
2011-09-14 04:31 PDT
,
Nayan Kumar K
no flags
Details
Formatted Diff
Diff
Title property added.
(13.59 KB, patch)
2011-09-29 12:39 PDT
,
Nayan Kumar K
no flags
Details
Formatted Diff
Diff
Title property added.
(18.81 KB, patch)
2011-09-30 04:26 PDT
,
Nayan Kumar K
no flags
Details
Formatted Diff
Diff
Title property added
(21.01 KB, patch)
2011-09-30 04:34 PDT
,
Nayan Kumar K
no flags
Details
Formatted Diff
Diff
Title property added to WebKitWebView.
(19.01 KB, patch)
2011-09-30 04:39 PDT
,
Nayan Kumar K
mrobinson
: review-
Details
Formatted Diff
Diff
Added title property
(15.69 KB, patch)
2011-10-03 00:47 PDT
,
Nayan Kumar K
no flags
Details
Formatted Diff
Diff
Added 'title' property
(15.83 KB, patch)
2011-10-03 02:07 PDT
,
Nayan Kumar K
no flags
Details
Formatted Diff
Diff
title property added
(16.64 KB, patch)
2011-10-03 02:47 PDT
,
Nayan Kumar K
no flags
Details
Formatted Diff
Diff
title property added
(14.58 KB, patch)
2011-10-04 05:22 PDT
,
Nayan Kumar K
mrobinson
: review-
gustavo
: commit-queue-
Details
Formatted Diff
Diff
Added 'title' property
(13.16 KB, patch)
2011-10-06 11:15 PDT
,
Nayan Kumar K
no flags
Details
Formatted Diff
Diff
title property added
(13.05 KB, patch)
2011-10-06 12:22 PDT
,
Nayan Kumar K
gustavo
: commit-queue-
Details
Formatted Diff
Diff
title property added
(1.08 KB, patch)
2011-10-07 02:20 PDT
,
Nayan Kumar K
no flags
Details
Formatted Diff
Diff
title property added
(1.41 KB, patch)
2011-10-07 02:23 PDT
,
Nayan Kumar K
no flags
Details
Formatted Diff
Diff
title property added
(13.68 KB, patch)
2011-10-07 02:24 PDT
,
Nayan Kumar K
gustavo.noronha
: commit-queue-
Details
Formatted Diff
Diff
title property added
(9.49 KB, patch)
2011-10-20 07:36 PDT
,
Nayan Kumar K
mrobinson
: review+
webkit.review.bot
: commit-queue-
Details
Formatted Diff
Diff
title property added
(10.12 KB, patch)
2011-10-20 23:53 PDT
,
Nayan Kumar K
mrobinson
: review+
gustavo
: commit-queue-
Details
Formatted Diff
Diff
Add 'title' property
(10.06 KB, patch)
2011-11-03 05:36 PDT
,
Nayan Kumar K
no flags
Details
Formatted Diff
Diff
Show Obsolete
(16)
View All
Add attachment
proposed patch, testcase, etc.
Nayan Kumar K
Comment 1
2011-09-14 04:31:07 PDT
Created
attachment 107318
[details]
Add title and progress properties
Nayan Kumar K
Comment 2
2011-09-14 04:35:40 PDT
With this patch, webkit_web_view_get_title returns g_strdup of the title obtained using title.utf8().data() (As directly returning title.utf8().data() returns garbage). Also it assumes that application calling this function is responsible for freeing this dup'ed memory, which is not consistent with the definition of this API in webkit1. One way to solve this would be to maintain title inside WebKitWebViewPrivate. Will that be a good approach to follow?
Nayan Kumar K
Comment 3
2011-09-29 12:39:32 PDT
Created
attachment 109188
[details]
Title property added. Implementation of title property as per the new GObjectization design. This approach adds 'title' property to WebKitWebView. Loader client will register didTitleReceiveCallback and intimates WebView upon title change. WebView will in turn notifies the application via g_object_notify. GtkLauncher2 is modified accordingly.
WebKit Review Bot
Comment 4
2011-09-29 12:43:04 PDT
Attachment 109188
[details]
did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit..." exit_code: 1 Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h:99: The parameter name "webView" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h:99: Extra space before ( in function call [whitespace/parens] [4] Tools/ChangeLog:1: ChangeLog entry has no bug number [changelog/bugnumber] [5] Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:154: Missing space before ( in if( [whitespace/parens] [5] Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:298: webkit_web_view_get_title is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:305: Missing space before ( in if( [whitespace/parens] [5] Source/WebKit2/UIProcess/API/gtk/WebKitWebLoaderClient.cpp:128: Declaration has space between type name and * in WebKitWebLoaderClient *loaderClient [whitespace/declaration] [3] Total errors found: 7 in 8 files If any of these errors are false positives, please file a bug against check-webkit-style.
Carlos Garcia Campos
Comment 5
2011-09-29 23:25:27 PDT
Comment on
attachment 109188
[details]
Title property added. View in context:
https://bugs.webkit.org/attachment.cgi?id=109188&action=review
Thanks for the patch!.
> Source/WebKit2/UIProcess/API/gtk/WebKitWebLoaderClient.cpp:132 > + webkitWebViewNotifyTitleReceived(webView, title); > +}
This callback should emit a signal like the other callbacks, and the view can just connect to it to update the title. That way we don't private API to notify the view.
> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:35 > +#include <wtf/text/WTFString.h>
You are using CString, so include CString.h instead.
> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:133 > + * WebKitWebView:title
This should end with :
> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:135 > + * The title associated with WebView's document.
Use the same documentation than webkit1 for things that don't change, this way we have the same strings for translators and API users know this is the same thing.
> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:150 > + g_return_if_fail(WEBKIT_IS_WEB_VIEW(webView));
Don't use g_return* macros in private methods.
> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:296 > + * Returns the #Object:title associated with #WebKitWebView.
It should be Returns: Use the same documentation than webkit1 here too.
> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:308 > + WebKitWebViewPrivate* priv = webView->priv; > + WebPageProxy* page = webkitWebViewBaseGetPage(WEBKIT_WEB_VIEW_BASE(webView)); > + String title = page->pageTitle(); > + if(priv->title != title.utf8()) > + priv->title = title.utf8(); > + > + return priv->title.data();
We are already notified when the title changes, we shouldn't need to check it here again. Or can it change after document is loaded?
> Tools/ChangeLog:10 > + * GtkLauncher/main.c: > + (updateTitle): > + (createBrowser): > +
I wouldn't change the GtkLauncher code. We added this GtkLauncher2 when we wanted to use the same api than webkit1, ideally the same code would work and current #ifdefs were just provisional until new api was added. That's not the case anymore, and we don't want GtkLauncher code with a lot of #ifdefs to support both wk1 and wk2. For now, it's better to add unit tests for every new API. Once we have enough API, we will port MiniBrowser to use the GTK+ API and we'll remove webkit2 support from GtkLauncher.
Nayan Kumar K
Comment 6
2011-09-29 23:57:57 PDT
Comment on
attachment 109188
[details]
Title property added. View in context:
https://bugs.webkit.org/attachment.cgi?id=109188&action=review
>> Source/WebKit2/UIProcess/API/gtk/WebKitWebLoaderClient.cpp:132 >> +} > > This callback should emit a signal like the other callbacks, and the view can just connect to it to update the title. That way we don't private API to notify the view.
If I understood correctly, this would mean that, applications can connect to 'title-change' signal on WebKitWebLoaderClient and can be notified when title changes. Also, since WebView in turn notifies about the change in 'title' property, application might get redundant notification if they connect both to 'title-change' signal on WebKitWebLoaderClient or 'notify:title' on WebView. I think, our API's should restrict redundancy. What do you say?
>> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:35 >> +#include <wtf/text/WTFString.h> > > You are using CString, so include CString.h instead.
I use String also, in webkit_web_view_get_title function.
>> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:308 >> + return priv->title.data(); > > We are already notified when the title changes, we shouldn't need to check it here again. Or can it change after document is loaded?
I think you are right, WebKit should notify if there are any change in the title (even after the document loads). I think its fair enough to assume that this function gets triggered only upon title change.
>> Tools/ChangeLog:10 >> + > > I wouldn't change the GtkLauncher code. We added this GtkLauncher2 when we wanted to use the same api than webkit1, ideally the same code would work and current #ifdefs were just provisional until new api was added. That's not the case anymore, and we don't want GtkLauncher code with a lot of #ifdefs to support both wk1 and wk2. For now, it's better to add unit tests for every new API. Once we have enough API, we will port MiniBrowser to use the GTK+ API and we'll remove webkit2 support from GtkLauncher.
Make sense. I will try to add unit test for this.
Nayan Kumar K
Comment 7
2011-09-30 04:26:49 PDT
Created
attachment 109281
[details]
Title property added. Incorporated review comments. Added unit test.
WebKit Review Bot
Comment 8
2011-09-30 04:29:42 PDT
Attachment 109281
[details]
did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit..." exit_code: 1 Source/WebKit2/ChangeLog:1: ChangeLog entry has no bug number [changelog/bugnumber] [5] Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h:99: The parameter name "webView" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h:99: Extra space before ( in function call [whitespace/parens] [4] Source/WebKit2/UIProcess/API/gtk/tests/testwebview.c:46: One line control clauses should not use braces. [whitespace/braces] [4] Source/WebKit2/UIProcess/API/gtk/tests/testwebview.c:182: Declaration has space between * and variable name in SoupServer* server [whitespace/declaration] [3] Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:295: webkit_web_view_get_title is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] Total errors found: 6 in 7 files If any of these errors are false positives, please file a bug against check-webkit-style.
Nayan Kumar K
Comment 9
2011-09-30 04:34:50 PDT
Created
attachment 109282
[details]
Title property added Fixed few style issues.
WebKit Review Bot
Comment 10
2011-09-30 04:37:12 PDT
Attachment 109282
[details]
did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit..." exit_code: 1 Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h:93: Extra space before ( in function call [whitespace/parens] [4] Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h:93: The parameter name "web_view" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h:104: The parameter name "webView" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h:104: Extra space before ( in function call [whitespace/parens] [4] Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:255: webkit_web_view_load_html_string is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:304: webkit_web_view_get_title is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] Total errors found: 6 in 7 files If any of these errors are false positives, please file a bug against check-webkit-style.
Nayan Kumar K
Comment 11
2011-09-30 04:39:57 PDT
Created
attachment 109283
[details]
Title property added to WebKitWebView. Oops, submitted wrong patch earlier!
WebKit Review Bot
Comment 12
2011-09-30 04:42:40 PDT
Attachment 109283
[details]
did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit..." exit_code: 1 Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h:99: The parameter name "webView" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h:99: Extra space before ( in function call [whitespace/parens] [4] Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:295: webkit_web_view_get_title is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] Total errors found: 3 in 7 files If any of these errors are false positives, please file a bug against check-webkit-style.
Martin Robinson
Comment 13
2011-10-01 11:30:33 PDT
Comment on
attachment 109283
[details]
Title property added to WebKitWebView. View in context:
https://bugs.webkit.org/attachment.cgi?id=109283&action=review
> Source/WebKit2/ChangeLog:8 > + Support for 'title' property is added in WebKitWebView. > + Functions to get the value of this property is provided. > +
Typically the description goes under the review line.
> Source/WebKit2/UIProcess/API/gtk/WebKitWebLoaderClient.cpp:131 > + webkitWebViewNotifyTitleReceived(webView, title);
This can just be webkitWebViewNotifyTitleReceived(WEBKIT_WEB_LOADER_CLIENT(clientInfo)->priv->view.get(), title);
> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:50 > WebKitWebContext* context; > > GRefPtr<WebKitWebLoaderClient> loaderClient; > + CString title;
Let's keep all the properties and loader clients in separate groups. The order isn't important beyond organization.
> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:134 > + * Returns: the @web_view's document title.
I think you should be more explicit here. This is the title of the main frame. also why are you using @web_view there's no parameter. I recommend using #WebKitWebView explicitly instead: Returns: the main frame title of this #WebKitWebView. If the title has not been received yet it will be an empty string.
> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:145 > +void webkitWebViewNotifyTitleReceived(WebKitWebView *webView, WKStringRef titleRef)
I'd prefer this to be called webkitWebViewSetTitle and to take a const char*.
>> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:295 >> + * Returns the @web_view's document title. >> + * >> + * Returns: the title of @web_view. >> + */ >> +const gchar* webkit_web_view_get_title(WebKitWebView* webView) > > webkit_web_view_get_title is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4]
Here you have the same problem but it's worse even becaues the name of the paramter is @webView. We really need a build step which verifies the sanity of the documentation. I'd like to have zero warnings when generating it.
> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:301 > + > + WebKitWebViewPrivate* priv = webView->priv; > + > + return priv->title.data();
This can all be one line. Ensure that an empty CString does not return null.
>>> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h:99 >>> +webkit_web_view_get_title (WebKitWebView *webView); >> >> The parameter name "webView" adds no information, so it should be removed. [readability/parameter_name] [5] > > Extra space before ( in function call [whitespace/parens] [4]
Shoudl be *web_view.
> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewPrivate.h:36 > +G_BEGIN_DECLS > + > +void webkitWebViewNotifyTitleReceived(WebKitWebView *, WKStringRef); > + > +G_END_DECLS
There's no need to specify that these are unmangled. In fact, it's probably better that private API stays mangled.
> Source/WebKit2/UIProcess/API/gtk/tests/testwebview.c:38 > +#define HTML_TITLE "WelCome to WebKit-GTK+" > +/* This string has to be rather big because of the cancelled test - it > + * looks like soup refuses to send or receive a too small chunk */ > +#define HTML_STRING "<html><head><title>"HTML_TITLE"</title></head><body>Testing!Testing!Testing!Testing!Testing!Testing!Testing!Testing!Testing!Testing!Testing!Testing!Testing!Testing!Testing!Testing!Testing!Testing!Testing!Testing!Testing!Testing!Testing!Testing!Testing!Testing!Testing!Testing!Testing!Testing!Testing!Testing!Testing!Testing!Testing!Testing!Testing!Testing!Testing!Testing!Testing!Testing!Testing!Testing!Testing!Testing!Testing!Testing!Testing!Testing!Testing!Testing!Testing!Testing!Testing!Testing!Testing!Testing!Testing!Testing!Testing!Testing!Testing!Testing!Testing!Testing!Testing!Testing!Testing!Testing!Testing!Testing!Testing!Testing!Testing!Testing!Testing!Testing!Testing!Testing!Testing!Testing!Testing!Testing!Testing!Testing!</body></html>" > + > +/* For real request testing */ > +static void serverCallback(SoupServer* server, SoupMessage* msg, const char* path, GHashTable* query, SoupClientContext* context, gpointer data) > +{ > + if (msg->method != SOUP_METHOD_GET) { > + soup_message_set_status(msg, SOUP_STATUS_NOT_IMPLEMENTED);
Please don't just copy and paste the entire loading test. The test could be quite simple you could do a notify on the title property of WebView, load a simple string and then assert that that the title is what you think it is. Don't forget to assert the proper title before loading anything too.
> Source/WebKit2/UIProcess/API/gtk/tests/testwebview.c:195 > + server = soup_server_new(SOUP_SERVER_PORT, 0, NULL); > + soup_server_run_async(server); > + > + soup_server_add_handler(server, NULL, serverCallback, NULL, NULL); > + > + baseURI = soup_uri_new("
http://127.0.0.1/
"); > + soup_uri_set_port(baseURI, soup_server_get_port(server));
I'm almost certain you don't need to make a SoupServer here.
Nayan Kumar K
Comment 14
2011-10-03 00:47:58 PDT
Created
attachment 109449
[details]
Added title property
WebKit Review Bot
Comment 15
2011-10-03 00:50:07 PDT
Attachment 109449
[details]
did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit..." exit_code: 1 Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h:99: The parameter name "web_view" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h:99: Extra space before ( in function call [whitespace/parens] [4] Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:298: webkit_web_view_get_title is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] Total errors found: 3 in 7 files If any of these errors are false positives, please file a bug against check-webkit-style.
Nayan Kumar K
Comment 16
2011-10-03 02:07:59 PDT
Created
attachment 109452
[details]
Added 'title' property Incorporated review comments.
Nayan Kumar K
Comment 17
2011-10-03 02:08:30 PDT
Comment on
attachment 109283
[details]
Title property added to WebKitWebView. View in context:
https://bugs.webkit.org/attachment.cgi?id=109283&action=review
>> Source/WebKit2/UIProcess/API/gtk/WebKitWebLoaderClient.cpp:131 >> + webkitWebViewNotifyTitleReceived(webView, title); > > This can just be webkitWebViewNotifyTitleReceived(WEBKIT_WEB_LOADER_CLIENT(clientInfo)->priv->view.get(), title);
Done
>> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:50 >> + CString title; > > Let's keep all the properties and loader clients in separate groups. The order isn't important beyond organization.
Done.
>> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:134 >> + * Returns: the @web_view's document title. > > I think you should be more explicit here. This is the title of the main frame. also why are you using @web_view there's no parameter. I recommend using #WebKitWebView explicitly instead: > > Returns: the main frame title of this #WebKitWebView. If the title has not been received yet it will be an empty string.
Done.
>> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:145 >> +void webkitWebViewNotifyTitleReceived(WebKitWebView *webView, WKStringRef titleRef) > > I'd prefer this to be called webkitWebViewSetTitle and to take a const char*.
Changed the signature from webkitWebViewNotifyTitleReceived to webkitWebViewSetTitle. However, if we change the argument to const char*, then we need to cache WebKit::toImpl(titleRef)->string().utf8().data(), send this as argument of webKitWebViewSetTitle and convert it back to CString in webkitWebViewSetTitle implementation, as we copy this variable to WebView property 'title', which is a CString. Since this is a private method, I believe cleaner way to do this is via WKStringRef. Please let me know your opinion.
>>> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:295 >>> +const gchar* webkit_web_view_get_title(WebKitWebView* webView) >> >> webkit_web_view_get_title is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] > > Here you have the same problem but it's worse even becaues the name of the paramter is @webView. We really need a build step which verifies the sanity of the documentation. I'd like to have zero warnings when generating it.
Done. Please note that, I tried to keep the documentation same as webkit1 (
https://bugs.webkit.org/show_bug.cgi?id=68074#c5
). In fact, few of the methods added earlier also refers to @web_view, hence I followed the same. I am in process of writing a patch to generate documentation for webkit2 gtk+ api, I will address rest of such mismatches in that patch.
>> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:301 >> + return priv->title.data(); > > This can all be one line. Ensure that an empty CString does not return null.
Done.
>>>> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h:99 >>>> +webkit_web_view_get_title (WebKitWebView *webView); >>> >>> The parameter name "webView" adds no information, so it should be removed. [readability/parameter_name] [5] >> >> Extra space before ( in function call [whitespace/parens] [4] > > Shoudl be *web_view.
Sorry, corrected it now.
>> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewPrivate.h:36 >> +G_END_DECLS > > There's no need to specify that these are unmangled. In fact, it's probably better that private API stays mangled.
Done.
>> Source/WebKit2/UIProcess/API/gtk/tests/testwebview.c:38 >> + soup_message_set_status(msg, SOUP_STATUS_NOT_IMPLEMENTED); > > Please don't just copy and paste the entire loading test. The test could be quite simple you could do a notify on the title property of WebView, load a simple string and then assert that that the title is what you think it is. Don't forget to assert the proper title before loading anything too.
Done, removed WebKitWebLoaderClient related tests from this patch, and added assert before loading anything for NULL title. Please note that, I added WebKitWebLoaderClient related test intentionally to test both title and loading (and assert if title is not received after LOAD_COMMIT and before LOAD_FINISH).
>> Source/WebKit2/UIProcess/API/gtk/tests/testwebview.c:195 >> + soup_uri_set_port(baseURI, soup_server_get_port(server)); > > I'm almost certain you don't need to make a SoupServer here.
I am afraid that I need to use soup_server, since we don't have the webkit_web_view_load_string functions yet. I have a patch for webkit_web_view_load_string function and will submit it for review soon. I submitted this patch first, since test for webkit_web_view_load_string uses webkit_web_view_get_title function.
WebKit Review Bot
Comment 18
2011-10-03 02:10:02 PDT
Attachment 109452
[details]
did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit..." exit_code: 1 Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h:99: The parameter name "web_view" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h:99: Extra space before ( in function call [whitespace/parens] [4] Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:304: webkit_web_view_get_title is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] Total errors found: 3 in 7 files If any of these errors are false positives, please file a bug against check-webkit-style.
Carlos Garcia Campos
Comment 19
2011-10-03 02:16:44 PDT
(In reply to
comment #17
)
> > Here you have the same problem but it's worse even becaues the name of the paramter is @webView. We really need a build step which verifies the sanity of the documentation. I'd like to have zero warnings when generating it. > > Done. > > Please note that, I tried to keep the documentation same as webkit1 (
https://bugs.webkit.org/show_bug.cgi?id=68074#c5
). In fact, few of the methods added earlier also refers to @web_view, hence I followed the same. I am in process of writing a patch to generate documentation for webkit2 gtk+ api, I will address rest of such mismatches in that patch.
It's web_view in public api, so I think the documentation should say web_view too.
> >> Source/WebKit2/UIProcess/API/gtk/tests/testwebview.c:38 > >> + soup_message_set_status(msg, SOUP_STATUS_NOT_IMPLEMENTED); > > > > Please don't just copy and paste the entire loading test. The test could be quite simple you could do a notify on the title property of WebView, load a simple string and then assert that that the title is what you think it is. Don't forget to assert the proper title before loading anything too. > > Done, removed WebKitWebLoaderClient related tests from this patch, and added assert before loading anything for NULL title. > > Please note that, I added WebKitWebLoaderClient related test intentionally to test both title and loading (and assert if title is not received after LOAD_COMMIT and before LOAD_FINISH).
Then I think you can just connect to title property in testwebview to check we have something when the document is loaded, and use existing loading tests in testloading to check the title is received between commit and finish (I guess this should be documented too)
Nayan Kumar K
Comment 20
2011-10-03 02:47:28 PDT
Created
attachment 109456
[details]
title property added Updated the patch
WebKit Review Bot
Comment 21
2011-10-03 02:51:11 PDT
Attachment 109456
[details]
did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit..." exit_code: 1 Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h:99: The parameter name "web_view" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h:99: Extra space before ( in function call [whitespace/parens] [4] Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:304: webkit_web_view_get_title is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] Total errors found: 3 in 7 files If any of these errors are false positives, please file a bug against check-webkit-style.
Nayan Kumar K
Comment 22
2011-10-04 05:22:44 PDT
Created
attachment 109609
[details]
title property added Changed the documentation of parameters back to @web_view, as gtk-doc uses header files for documenting declarations.
WebKit Review Bot
Comment 23
2011-10-04 05:26:02 PDT
Attachment 109609
[details]
did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit..." exit_code: 1 Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h:99: The parameter name "web_view" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h:99: Extra space before ( in function call [whitespace/parens] [4] Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:306: webkit_web_view_get_title is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] Total errors found: 3 in 6 files If any of these errors are false positives, please file a bug against check-webkit-style.
Gustavo Noronha (kov)
Comment 24
2011-10-04 10:37:18 PDT
Comment on
attachment 109609
[details]
title property added
Attachment 109609
[details]
did not pass gtk-ews (gtk): Output:
http://queues.webkit.org/results/9938480
Martin Robinson
Comment 25
2011-10-04 16:01:59 PDT
Comment on
attachment 109609
[details]
title property added View in context:
https://bugs.webkit.org/attachment.cgi?id=109609&action=review
Looking good in general, but we really need to be careful not to bloat our testing code. I think my comments below will prevent you from copying a lot of code from the loading test. In cases where we do need very similar code we should be abstracting out into helper functions and classes. Remeber that comments in WebKit should be full sentences that end with a period. Sometimes we miss instances of that, but the style guide is pretty clear about it. I think the only exception is if you are labeling some anonymous parameter.
> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:53 > WebKitWebContext* context; > > + /* Loader Client */ > GRefPtr<WebKitWebLoaderClient> loaderClient; > + > + /* WebKitWebView properties */ > + CString title;
Apologies. I don't think my comment was very clear upon re-reading it. Essentially I just suggested that you keep properties and client in separate blocks like: WebKitWebContext* context; CString title; ...other properties here later... I think you can omit the comments altogether. GRefPtr<WebKitWebLoaderClient> loaderClient; ...other clients here later...
> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:59 > + _WebKitWebViewPrivate() > + { > + /* Initialize the tile to empty string */ > + title = CString(""); > + }
In WebKit we always use initializer fields. No need for a comment. I think you can remove this also (see below).
> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:143 > + * Returns: the main frame title of this #WebKitWebView. If the title has not been received yet, it will be an empty string.
Sorry to nitpick again. :) Please check that the length of this line is consistent with the other documentation. Also, I'm pretty sure that properties don't use the "Returns" keyword. You should double-check with other GTK+ doc though.
> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:149 > + _("Returns the main frame title of #WebKitWebView"),
I'd rephrase to say "Main frame document title"
> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:151 > + "", > + static_cast<GParamFlags>(WEBKIT_PARAM_READABLE)));
Can't you add G_PARAM_CONSTRUCT_ONLY and avoid haivng a _WebKitWebViewPrivate constuctor above?
> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:154 > +void webkitWebViewSetTitle(WebKitWebView *webView, WKStringRef titleRef)
You asterisk is in the wrong place here. Please use const char for the title and convert in the caller.
> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:304 > + * Returns the property #Object:title. > + * > + * Returns: the main frame title of @web_view.
Hrm. It's a bit funky to refer to the property here. I think what we want to do is if the text is just a little blurb, reproduce it in both the property and getter and setter. If there's a longer description, have it in the getter. In the property documentation we probably just want to have a link that says "See webkit_web_view_get_title" I don't think you need to have a line that says "Returns" twice. Please say "the main frame document title..."
> Source/WebKit2/UIProcess/API/gtk/tests/testwebview.c:31 > +#define HTML_TITLE "WelCome to WebKit-GTK+" > +/* This string has to be rather big because of the cancelled test - it > + * looks like soup refuses to send or receive a too small chunk */
This comment indicates that you don't need the large string below...there is no cancelled test here. We don't need the huge HTML string. Additionally, I really want these strings not be defined as preprocessor constants. In WebKit we always prefer using static const char* for things like this. They should also be defined in the tightest scope possible.
> Source/WebKit2/UIProcess/API/gtk/tests/testwebview.c:48 > +/* For real request testing */ > +static void serverCallback(SoupServer* server, SoupMessage* msg, const char* path, GHashTable* query, SoupClientContext* context, gpointer data) > +{ > + if (msg->method != SOUP_METHOD_GET) { > + soup_message_set_status(msg, SOUP_STATUS_NOT_IMPLEMENTED); > + return; > + } > + > + soup_message_set_status(msg, SOUP_STATUS_OK); > + > + if (g_str_equal(path, "/")) > + soup_message_body_append(msg->response_body, SOUP_MEMORY_STATIC, HTML_STRING, strlen(HTML_STRING)); > + > + soup_message_body_complete(msg->response_body); > +}
Instead of doing this, let's wait until the load_html patch lands and avoid having to use SoupServer.
> Source/WebKit2/UIProcess/API/gtk/tests/testwebview.c:101 > + /* Assert if title is not empty */ > + g_assert_cmpstr(webkit_web_view_get_title(fixture->webView), ==, "");
I think you can just omit the comment here. If you feel the code is not explicit enough, feel free to be more explicit at the expense of redudancy. For instance: g_assert_cmpstr(webkit_web_view_get_title(fixture->webView), !=, 0); g_assert_cmpstr(webkit_web_view_get_title(fixture->webView), ==, "");
> Source/WebKit2/UIProcess/API/gtk/tests/testwebview.c:110 > + /* Assert if title is not received */ > + g_assert(fixture->hasReceivedTitle);
Instead of storing this information in the fixture, why not just assert that the title is correct here.
Carlos Garcia Campos
Comment 26
2011-10-04 23:20:59 PDT
(In reply to
comment #25
)
> > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:151 > > + "", > > + static_cast<GParamFlags>(WEBKIT_PARAM_READABLE))); > > Can't you add G_PARAM_CONSTRUCT_ONLY and avoid haivng a _WebKitWebViewPrivate constuctor above?
It should be CONSTRUCT, not CONSTRUCT_ONLY, but that would be a workaround to avoid initializing the attribute. The title can't be a constructor property because it's not writable, and you are not supposed to be able to create a view with a title. Properties with no value should not return a "", but NULL.
Carlos Garcia Campos
Comment 27
2011-10-06 06:02:32 PDT
Comment on
attachment 109609
[details]
title property added View in context:
https://bugs.webkit.org/attachment.cgi?id=109609&action=review
> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:157 > + CString title = WebKit::toImpl(titleRef)->string().utf8();
Use const CString& to avoid unnecessary string duplication.
Martin Robinson
Comment 28
2011-10-06 08:40:51 PDT
Comment on
attachment 109609
[details]
title property added View in context:
https://bugs.webkit.org/attachment.cgi?id=109609&action=review
>> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:157 >> + CString title = WebKit::toImpl(titleRef)->string().utf8(); > > Use const CString& to avoid unnecessary string duplication.
Hrm. I think that string() is a temporary so this might lead to a reference to a deleted object.
Nayan Kumar K
Comment 29
2011-10-06 11:15:17 PDT
Created
attachment 109980
[details]
Added 'title' property Review comments incorporated.
WebKit Review Bot
Comment 30
2011-10-06 11:17:50 PDT
Attachment 109980
[details]
did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit..." exit_code: 1 Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h:106: The parameter name "web_view" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h:106: Extra space before ( in function call [whitespace/parens] [4] Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:327: webkit_web_view_get_title is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] Total errors found: 3 in 7 files If any of these errors are false positives, please file a bug against check-webkit-style.
Nayan Kumar K
Comment 31
2011-10-06 11:28:20 PDT
Comment on
attachment 109609
[details]
title property added View in context:
https://bugs.webkit.org/attachment.cgi?id=109609&action=review
>> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:53 >> + CString title; > > Apologies. I don't think my comment was very clear upon re-reading it. Essentially I just suggested that you keep properties and client in separate blocks like: > > WebKitWebContext* context; > CString title; > ...other properties here later... > > I think you can omit the comments altogether. > > GRefPtr<WebKitWebLoaderClient> loaderClient; > ...other clients here later...
Done.
>> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:143 >> + * Returns: the main frame title of this #WebKitWebView. If the title has not been received yet, it will be an empty string. > > Sorry to nitpick again. :) Please check that the length of this line is consistent with the other documentation. Also, I'm pretty sure that properties don't use the "Returns" keyword. You should double-check with other GTK+ doc though.
Yes, gtk-doc does't have Return keyword for properties. Changed it new patch.
>> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:149 >> + _("Returns the main frame title of #WebKitWebView"), > > I'd rephrase to say "Main frame document title"
Ok.
>> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:151 >> + static_cast<GParamFlags>(WEBKIT_PARAM_READABLE))); > > Can't you add G_PARAM_CONSTRUCT_ONLY and avoid haivng a _WebKitWebViewPrivate constuctor above?
Since the property is readable only, we do not have webkit_web_view_set_function and hence using G_PARAM_CONSTRUCT will not be useful. Moreover, in the last uploaded patch, I am returning 0 if the title has not be received. This is how webkit1 works and I (and Carlos) think we should be consistent with webkit1 API's. Hence we do not need to have the initialization.
>> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:154 >> +void webkitWebViewSetTitle(WebKitWebView *webView, WKStringRef titleRef) > > You asterisk is in the wrong place here. Please use const char for the title and convert in the caller.
If we change the argument to const char*, then we need to cache WebKit::toImpl(titleRef)->string().utf8().data(), send this as argument of webKitWebViewSetTitle and convert it back to CString in webkitWebViewSetTitle implementation, as we copy this variable to WebView property 'title', which is a CString. Since this is a private method, I believe cleaner way to do this is via WKStringRef. (Comment no #17)
>> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:304 >> + * Returns: the main frame title of @web_view. > > Hrm. It's a bit funky to refer to the property here. I think what we want to do is if the text is just a little blurb, reproduce it in both the property and getter and setter. If there's a longer description, have it in the getter. In the property documentation we probably just want to have a link that says "See webkit_web_view_get_title" > > I don't think you need to have a line that says "Returns" twice. Please say "the main frame document title..."
Ok.
>> Source/WebKit2/UIProcess/API/gtk/tests/testwebview.c:31 >> + * looks like soup refuses to send or receive a too small chunk */ > > This comment indicates that you don't need the large string below...there is no cancelled test here. We don't need the huge HTML string. Additionally, I really want these strings not be defined as preprocessor constants. In WebKit we always prefer using static const char* for things like this. They should also be defined in the tightest scope possible.
I have reduced the length of HTML_STRING in latest uploaded patch. However, I still use these as preprocessor macros, as I need them in 2 different functions. I guess, it makes more sense to make it as preprocessor macro rather than using it as 'static const char *' in two different functions for this test.
>> Source/WebKit2/UIProcess/API/gtk/tests/testwebview.c:48 >> +} > > Instead of doing this, let's wait until the load_html patch lands and avoid having to use SoupServer.
Done. latest patch uses webkit_web_view_load_html and compares the title received.
>> Source/WebKit2/UIProcess/API/gtk/tests/testwebview.c:101 >> + g_assert_cmpstr(webkit_web_view_get_title(fixture->webView), ==, ""); > > I think you can just omit the comment here. If you feel the code is not explicit enough, feel free to be more explicit at the expense of redudancy. For instance: > > g_assert_cmpstr(webkit_web_view_get_title(fixture->webView), !=, 0); > g_assert_cmpstr(webkit_web_view_get_title(fixture->webView), ==, "");
Removed the comment.
>> Source/WebKit2/UIProcess/API/gtk/tests/testwebview.c:110 >> + g_assert(fixture->hasReceivedTitle); > > Instead of storing this information in the fixture, why not just assert that the title is correct here.
Done.
Martin Robinson
Comment 32
2011-10-06 11:43:11 PDT
(In reply to
comment #31
) =
> >> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:154 > >> +void webkitWebViewSetTitle(WebKitWebView *webView, WKStringRef titleRef) > > > > You asterisk is in the wrong place here. Please use const char for the title and convert in the caller. > > If we change the argument to const char*, then we need to cache WebKit::toImpl(titleRef)->string().utf8().data(), send this as argument of webKitWebViewSetTitle and convert it back to CString in webkitWebViewSetTitle implementation, as we copy this variable to WebView property 'title', which is a CString. Since this is a private method, I believe cleaner way to do this is via WKStringRef. (Comment no #17)
The issue with using WKStringRef here means that if we want to set the title in another place in the future we need to convert to a WKStringRef. I think the fact that the title was originally a WKStringRef is an implementation detail of the caller. If you are concerned about conversion from CString to const char and then back to CString feel free to pass a "const CString&". I do not prefer using WKStringRef here.
> > >> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:304 > >> + * Returns: the main frame title of @web_view. > > > > Hrm. It's a bit funky to refer to the property here. I think what we want to do is if the text is just a little blurb, reproduce it in both the property and getter and setter. If there's a longer description, have it in the getter. In the property documentation we probably just want to have a link that says "See webkit_web_view_get_title" > > > > I don't think you need to have a line that says "Returns" twice. Please say "the main frame document title..." > > Ok. > > >> Source/WebKit2/UIProcess/API/gtk/tests/testwebview.c:31 > >> + * looks like soup refuses to send or receive a too small chunk */ > > > > This comment indicates that you don't need the large string below...there is no cancelled test here. We don't need the huge HTML string. Additionally, I really want these strings not be defined as preprocessor constants. In WebKit we always prefer using static const char* for things like this. They should also be defined in the tightest scope possible. > > I have reduced the length of HTML_STRING in latest uploaded patch. However, I still use these as preprocessor macros, as I need them in 2 different functions. I guess, it makes more sense to make it as preprocessor macro rather than using it as 'static const char *' in two different functions for this test.
The biggest issue is that in WebKit we do not use #define, we use static const char*. If you like you can leave it at the global scope. Another option is to have it in main and pass it as the data argument to the tests. I'm going to try to land a unit test overhaul soon, by the way:
https://bugs.webkit.org/show_bug.cgi?id=69409
Nayan Kumar K
Comment 33
2011-10-06 12:22:29 PDT
Created
attachment 109993
[details]
title property added Updated as per review comments.
WebKit Review Bot
Comment 34
2011-10-06 12:28:43 PDT
Attachment 109993
[details]
did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit..." exit_code: 1 Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h:106: The parameter name "web_view" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h:106: Extra space before ( in function call [whitespace/parens] [4] Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:326: webkit_web_view_get_title is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] Total errors found: 3 in 7 files If any of these errors are false positives, please file a bug against check-webkit-style.
Gustavo Noronha (kov)
Comment 35
2011-10-06 14:51:31 PDT
Comment on
attachment 109993
[details]
title property added
Attachment 109993
[details]
did not pass gtk-ews (gtk): Output:
http://queues.webkit.org/results/9978344
Carlos Garcia Campos
Comment 36
2011-10-06 23:32:09 PDT
Comment on
attachment 109993
[details]
title property added View in context:
https://bugs.webkit.org/attachment.cgi?id=109993&action=review
This doesn't build, I wonder why it applies, because of the changes in current git master.
> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:144 > + static_cast<GParamFlags>(WEBKIT_PARAM_READABLE)));
WEBKIT_PARAM_READABLE si already casted, you don't need another cast here.
> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:363 > + * The main frame document title. If the title has not been received yet, > + * then NULL will be returned. > + * > + * Returns: The main frame title of @web_view.
I prefer something like this: * Gets the value of #WebKitWebView:title. * You can connect to ::notify signal of @web_view to * be notified when the title has been received. * * Returns: The main frame document title of @web_view, or %NULL if the title has not been received yet. Or something like that. This way you link to the property documentation and user knows that the property can be watched to be notified title changes.
> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h:114 > +WK_EXPORT const gchar *
Use WEBKIT_API instead WK_EXPORT, just update to current git master and rebase.
> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewPrivate.h:31 > +#include "WebKitWebView.h" > +
Don't you need to include wtf/text/CString.h here for CString?
> Source/WebKit2/UIProcess/API/gtk/tests/testwebview.c:37 > +typedef struct { > + WebKitWebView *webView; > + GMainLoop *loop; > +} WebViewTitleFixture;
Rename it to somethjing more generic, WEbViewFixture, so that it can be used by other tests too.
> Source/WebKit2/UIProcess/API/gtk/tests/testwebview.c:42 > + const gchar *title = webkit_web_view_get_title(webView); > + g_assert_cmpstr(title, ==, "Welcome to WebKitGtk+!");
This could be just g_assert_cmpstr(webkit_web_view_get_title(webView), ==, "Welcome to WebKitGtk+!");
> Source/WebKit2/UIProcess/API/gtk/tests/testwebview.c:50 > + g_signal_connect(fixture->webView, "notify::title", G_CALLBACK(webViewNotifyTitle), fixture);
Connect to the signal in testWebViewTitle since the fixture might be used for other tests.
> Source/WebKit2/UIProcess/API/gtk/tests/testwebview.c:57 > + g_assert_cmpstr(webkit_web_view_get_title(fixture->webView), ==, 0);
To check it's null just g_assert(!webkit_web_view_get_title(fixture->webView));
Martin Robinson
Comment 37
2011-10-06 23:36:55 PDT
Comment on
attachment 109993
[details]
title property added View in context:
https://bugs.webkit.org/attachment.cgi?id=109993&action=review
Looking good!
> Source/WebKit2/UIProcess/API/gtk/WebKitWebLoaderClient.cpp:126 > +static void didReceiveTitleForFrame(WKPageRef page, WKStringRef titleRef, WKFrameRef frameRef, WKTypeRef userData, const void* clientInfo)
userData is unused, so you should omit the name of the parameter.
> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:135 > + * The main frame title of this #WebKitWebView. If the title
You should also say "main frame document" here.
Carlos Garcia Campos
Comment 38
2011-10-06 23:39:38 PDT
Comment on
attachment 109993
[details]
title property added View in context:
https://bugs.webkit.org/attachment.cgi?id=109993&action=review
> Source/WebKit2/UIProcess/API/gtk/WebKitWebLoaderClient.cpp:131 > + const CString title = WebKit::toImpl(titleRef)->string().utf8();
I think you don't need WebKit:: there.
Nayan Kumar K
Comment 39
2011-10-07 02:20:58 PDT
Created
attachment 110108
[details]
title property added
Nayan Kumar K
Comment 40
2011-10-07 02:23:18 PDT
Created
attachment 110109
[details]
title property added
Nayan Kumar K
Comment 41
2011-10-07 02:24:42 PDT
Created
attachment 110110
[details]
title property added
Collabora GTK+ EWS bot
Comment 42
2011-10-07 02:29:05 PDT
Comment on
attachment 110110
[details]
title property added
Attachment 110110
[details]
did not pass gtk-ews (gtk): Output:
http://queues.webkit.org/results/10000152
Nayan Kumar K
Comment 43
2011-10-07 02:35:56 PDT
(In reply to
comment #42
)
> (From update of
attachment 110110
[details]
) >
Attachment 110110
[details]
did not pass gtk-ews (gtk): > Output:
http://queues.webkit.org/results/10000152
This patch depends on
https://bugs.webkit.org/show_bug.cgi?id=69249
which introduces webkit_web_view_load_html API.
Carlos Garcia Campos
Comment 44
2011-10-19 09:16:15 PDT
Comment on
attachment 110110
[details]
title property added View in context:
https://bugs.webkit.org/attachment.cgi?id=110110&action=review
This patch needs to be updated to use the new unit tests system, and it will need to be updated when
bug #69814
is fixed too.
> Source/WebKit2/UIProcess/API/gtk/WebKitWebLoaderClient.cpp:147 > + const CString title = toImpl(titleRef)->string().utf8(); > + webkitWebViewSetTitle(WEBKIT_WEB_LOADER_CLIENT(clientInfo)->priv->view.get(), title);
I think we can just avoid caching title here and use a single line webkitWebViewSetTitle(WEBKIT_WEB_LOADER_CLIENT(clientInfo)->priv->view.get(), toImpl(titleRef)->string().utf8());
> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:136 > + * the title has not been received yet, it will be NULL.
Use %NULL.
Nayan Kumar K
Comment 45
2011-10-20 07:36:57 PDT
Created
attachment 111769
[details]
title property added
WebKit Review Bot
Comment 46
2011-10-20 08:03:19 PDT
Comment on
attachment 111769
[details]
title property added
Attachment 111769
[details]
did not pass chromium-ews (chromium-xvfb): Output:
http://queues.webkit.org/results/10182421
New failing tests: http/tests/inspector/resource-tree/resource-tree-document-url.html
Collabora GTK+ EWS bot
Comment 47
2011-10-20 09:22:01 PDT
Comment on
attachment 111769
[details]
title property added
Attachment 111769
[details]
did not pass gtk-ews (gtk): Output:
http://queues.webkit.org/results/10179468
Martin Robinson
Comment 48
2011-10-20 23:10:03 PDT
Any clue why this broke the build?
Martin Robinson
Comment 49
2011-10-20 23:44:16 PDT
Comment on
attachment 111769
[details]
title property added View in context:
https://bugs.webkit.org/attachment.cgi?id=111769&action=review
> Source/WebKit2/UIProcess/API/gtk/WebKitWebLoaderClient.cpp:126 > +static void didReceiveTitleForFrame(WKPageRef page, WKStringRef titleRef, WKFrameRef frameRef, WKTypeRef, const void* clientInfo)
You can remove the name of clientInfo, since it's unused.
Nayan Kumar K
Comment 50
2011-10-20 23:53:19 PDT
Created
attachment 111914
[details]
title property added Incorporated review comments.
Gustavo Noronha (kov)
Comment 51
2011-10-21 00:04:26 PDT
Comment on
attachment 111914
[details]
title property added
Attachment 111914
[details]
did not pass gtk-ews (gtk): Output:
http://queues.webkit.org/results/10176816
Carlos Garcia Campos
Comment 52
2011-11-03 04:53:27 PDT
Comment on
attachment 111914
[details]
title property added This is already r+, but it still needs some things before landing: - Make sure it still applies to current git master, I think it doesn't - Add new symbols to gtk-doc sections file - Update the unit test to use WebViewTest::LoadHtml() instead of webkit_web_view_load_html() directly
Nayan Kumar K
Comment 53
2011-11-03 05:36:53 PDT
Created
attachment 113468
[details]
Add 'title' property Changes included in this patch are, a). Rebased the patch to latest webkit trunk. b). Use WebViewTest::loadHtml instead of webkit_web_view_load_html. c). Added 'webkit_web_view_get_title' in webkit2gtk-sections.txt file.
WebKit Review Bot
Comment 54
2011-11-03 05:40:05 PDT
Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See
http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API
Carlos Garcia Campos
Comment 55
2011-11-03 05:52:17 PDT
Comment on
attachment 113468
[details]
Add 'title' property LGTM
Philippe Normand
Comment 56
2011-11-03 06:12:16 PDT
Comment on
attachment 113468
[details]
Add 'title' property Thanks!
WebKit Review Bot
Comment 57
2011-11-03 06:26:54 PDT
Comment on
attachment 113468
[details]
Add 'title' property Clearing flags on attachment: 113468 Committed
r99179
: <
http://trac.webkit.org/changeset/99179
>
WebKit Review Bot
Comment 58
2011-11-03 06:27:03 PDT
All reviewed patches have been landed. Closing bug.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug