[GTK] Click on an anchor link makes page title dissappear

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Manuel Rego Casasnovas]

Created attachment 182776 [details]
Example HTML file to reproduce the issue

Overview: In GtkLauncher if you go to a page with anchors and you click in one of them, the page title is set to NULL and you only see the text " - WebKit Launcher"

Steps to Reproduce:
1) Open the attached HTML page in GtkLauncher. Page title is "My page title - WebKit Launcher"
2) Click on the link with the text "this anchor link"

Actual Results: Page title is set to " - WebKit Launcher"

Expected Results: Page title is still "My page title - WebKit Launcher"

[Manuel Rego Casasnovas]

Created attachment 182779 [details]
Patch

[Martin Robinson]

Comment on attachment 182779 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=182779&action=review

> Source/WebKit/gtk/WebCoreSupport/FrameLoaderClientGtk.cpp:856
> -    priv->title = NULL;
> +    priv->title = g_strdup(core(m_frame)->loader()->activeDocumentLoader()->title().string().utf8().data());

I'm not sure this is valid. The new title isn't necessarily available when a new load is committed. There's another FrameLoader messages that deals with titles. Perhaps what this is doing is resetting the title to the original value? it also makes sense to check the WebKit2 behavior.

[Manuel Rego Casasnovas]

(In reply to comment #2)
> (From update of attachment 182779 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=182779&action=review
> 
> > Source/WebKit/gtk/WebCoreSupport/FrameLoaderClientGtk.cpp:856
> > -    priv->title = NULL;
> > +    priv->title = g_strdup(core(m_frame)->loader()->activeDocumentLoader()->title().string().utf8().data());
> 
> I'm not sure this is valid. The new title isn't necessarily available when a new load is committed. There's another FrameLoader messages that deals with titles. Perhaps what this is doing is resetting the title to the original value? it also makes sense to check the WebKit2 behavior.

I agree that the title could not be available at this point yet, but in these cases FrameLoaderClient::dispatchDidReceiveTitle will be called when it is available and the title will be updated accordingly.

The problem is in this particular case. When you click on an anchor, FrameLoaderClient::dispatchDidCommitLoad is called from FrameLoaderClient::dispatchDidNavigateWithinPage. Then FrameLoaderClient::dispatchDidCommitLoad resets title to NULL, however FrameLoaderClient::dispatchDidReceiveTitle is not called (as title is not received again).

About how it works in WK2, it's quite different as when you click on an anchor the method WebPageProxy::didSameDocumentNavigationForFrame is called. And it requests to update the URL but not the page title, moreover it doesn't reset the title property either.

Not sure about the best approach to fix the issue, any idea is welcomed :-)

[Martin Robinson]

The right thing to do here seems to be to to not set the title to NULL for navigation within the page.

[Manuel Rego Casasnovas]

Created attachment 184184 [details]
Patch

[Martin Robinson]

Comment on attachment 184184 [details]
Patch

Instead of adding new state to the FrameLoaderClient, wouldn't it be simpler to abstract the shared logic of dispatchDidCommitLoad into a helper method and simply keep clearing the title in dispatchDidCommitLoad?

[Manuel Rego Casasnovas]

Created attachment 184239 [details]
Patch

[Martin Robinson]

Comment on attachment 184239 [details]
Patch

Okay. Looks good. Did you run the unit tests? In the future you probably want to avoid the usage of booleans [1], but for this change it's probably okay.

1. See the WebKit style guide where it says: "Prefer enums to bools on function parameters if callers are likely to be passing constants, since named constants are easier to read at the call site. An exception to this rule is a setter function, where the name of the function already makes clear what the boolean is."

[Manuel Rego Casasnovas]

(In reply to comment #8)
> (From update of attachment 184239 [details])
> Okay. Looks good. Did you run the unit tests?

Yes, they gave me the same results than without my patch.

>In the future you probably want to avoid the usage of booleans [1], but for this change it's probably okay.

I agree that it's better to use enums, I've read the coding style guidelines but I didn't remember all of them yet :-) Anyway in this case, for a private method I think it's not so important as when you have a public method that could be called from somewhere else.

I can include a new enum if you prefer. Let me know.

[WebKit Review Bot]

Comment on attachment 184239 [details]
Patch

Clearing flags on attachment: 184239

Committed r140557: <http://trac.webkit.org/changeset/140557>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.