Summary: | [GTK] Inconsistent state of WebKitWebView when replacing content in WebKit2 | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Carlos Garcia Campos <cgarcia> | ||||
Component: | WebKit2 | Assignee: | Nobody <webkit-unassigned> | ||||
Status: | RESOLVED FIXED | ||||||
Severity: | Normal | CC: | gustavo, pnormand, webkit.review.bot | ||||
Priority: | P2 | Keywords: | Gtk | ||||
Version: | 528+ (Nightly build) | ||||||
Hardware: | PC | ||||||
OS: | Linux | ||||||
Bug Depends on: | |||||||
Bug Blocks: | 79777 | ||||||
Attachments: |
|
Description
Carlos Garcia Campos
2012-02-28 03:46:16 PST
Created attachment 129225 [details]
Patch
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 Comment on attachment 129225 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=129225&action=review Looks good. Please consider the minor cleanups below. > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:82 > + NoReplaceContent, NotReplacingContent? > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:703 > - if (webView->priv->replacingContent) { > + if (webView->priv->replaceContentStatus == ReplacingContent) { > if (loadEvent == WEBKIT_LOAD_FINISHED) > - webView->priv->replacingContent = false; > + webView->priv->replaceContentStatus = DidReplaceContent; > return; > } > > + if (loadEvent == WEBKIT_LOAD_STARTED) { > + if (webView->priv->replaceContentStatus == WillReplaceContent) { > + webView->priv->replaceContentStatus = ReplacingContent; > + return; > + } > + webView->priv->replaceContentStatus = NoReplaceContent; > + } Mind moving this to a helper called updateReplaceContentStatus? > Source/WebKit2/UIProcess/API/gtk/tests/WebViewTest.cpp:148 > +void WebViewTest::waitUntilTitleChanged(const char* expectedTitle) Probably better to call this waitUntilTitleChangedTo. (In reply to comment #3) > (From update of attachment 129225 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=129225&action=review > > Looks good. Please consider the minor cleanups below. > > > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:82 > > + NoReplaceContent, > > NotReplacingContent? Sounds good. > > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:703 > > - if (webView->priv->replacingContent) { > > + if (webView->priv->replaceContentStatus == ReplacingContent) { > > if (loadEvent == WEBKIT_LOAD_FINISHED) > > - webView->priv->replacingContent = false; > > + webView->priv->replaceContentStatus = DidReplaceContent; > > return; > > } > > > > + if (loadEvent == WEBKIT_LOAD_STARTED) { > > + if (webView->priv->replaceContentStatus == WillReplaceContent) { > > + webView->priv->replaceContentStatus = ReplacingContent; > > + return; > > + } > > + webView->priv->replaceContentStatus = NoReplaceContent; > > + } > > Mind moving this to a helper called updateReplaceContentStatus? Sure > > Source/WebKit2/UIProcess/API/gtk/tests/WebViewTest.cpp:148 > > +void WebViewTest::waitUntilTitleChanged(const char* expectedTitle) > > Probably better to call this waitUntilTitleChangedTo. I didn't add the 'To' because you can pass NULL to wait until title is changed to whatever thing. maybe it's better to use a default value of NULL, though Comment on attachment 129225 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=129225&action=review >>> Source/WebKit2/UIProcess/API/gtk/tests/WebViewTest.cpp:148 >>> +void WebViewTest::waitUntilTitleChanged(const char* expectedTitle) >> >> Probably better to call this waitUntilTitleChangedTo. > > I didn't add the 'To' because you can pass NULL to wait until title is changed to whatever thing. maybe it's better to use a default value of NULL, though You could add another method called waitUntilTitleChanged that just calls waitUntilTitleChangedTo with a NULL parameter. Committed r109120: <http://trac.webkit.org/changeset/109120> |