Bug 33428 – [GTK] couple fixes for signal emissions, and property notifications

• Status: RESOLVED
• Resolution: FIXED
• Component: Page Loading

[Gustavo Noronha (kov)]

Our API is a bit messy where load status is concerned, specially since error pages were introduced, because they cause their own load statuses to be emitted. I have a couple of patches to propose, to fix these issues.

[Gustavo Noronha (kov)]

Created attachment 46215 [details]
do not call committedLoad on finishedLoading

I made sure tests pass, unit tests pass, and Epiphany behaviour stays the same. This, along with the second patch will allow us to remove a number of work-arounds from Epiphany, though.

[WebKit Review Bot]

Attachment 46215 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebKit/gtk/WebCoreSupport/FrameLoaderClientGtk.cpp:844:  Declaration has space between type name and * in FrameLoader *loader  [whitespace/declaration] [3]
Total errors found: 1

[Gustavo Noronha (kov)]

Created attachment 46216 [details]
avoid emitting load signals for error pages loads

ChangeLog entry explains why. We can remove most of the workarounds we currently have in ephy with this.

[Gustavo Noronha (kov)]

Created attachment 46217 [details]
do not call committedLoad on finishedLoading

[WebKit Review Bot]

Attachment 46217 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebKit/gtk/WebCoreSupport/FrameLoaderClientGtk.cpp:149:  Declaration has space between type name and * in FrameLoader *frameLoader  [whitespace/declaration] [3]
WebKit/gtk/WebCoreSupport/FrameLoaderClientGtk.cpp:844:  Declaration has space between type name and * in FrameLoader *loader  [whitespace/declaration] [3]
Total errors found: 2

[Adam Barth]

Why does this patch intentionally move the *?

-          FrameLoader* frameLoader = loader->frameLoader();
+         FrameLoader *frameLoader = loader->frameLoader();

That seems like a violation of the style guide...

[Gustavo Noronha (kov)]

(In reply to comment #6)
> Why does this patch intentionally move the *?
> 
> -          FrameLoader* frameLoader = loader->frameLoader();
> +         FrameLoader *frameLoader = loader->frameLoader();
> 
> That seems like a violation of the style guide...

Yeah, I saw the warning and was apparently too sleepy to make the correct fix =);

[Gustavo Noronha (kov)]

Created attachment 46232 [details]
do not call committedLoad on finishedLoading

Hopefully this one has not style issues.

[Xan Lopez]

Comment on attachment 46232 [details]
do not call committedLoad on finishedLoading

OK.

[Gustavo Noronha (kov)]

Created attachment 46269 [details]
avoid emitting load status signals when loading error pages

Aha, that's what happens when you kill changes and have to reimplement them. The patch had an issue. This fixes it.

[Gustavo Noronha (kov)]

Created attachment 46271 [details]
epiphany patch to remove workarounds

This is what we can do in Ephy.

[Gustavo Noronha (kov)]

Comment on attachment 46232 [details]
do not call committedLoad on finishedLoading

Landed as r53075.

[Gustavo Noronha (kov)]

I ended up rolling back the committedLoad fix, since it caused a couple ASSERTS I cannot reproduce locally to be hit. I'll investigate a bit more.

[Eric Seidel (no email)]

The rollout does not seem to have stopped the breakage:
http://build.webkit.org/results/GTK%20Linux%2032-bit%20Debug/r53085%20(2279)/plugins/iframe-shims-stderr.txt

[Eric Seidel (no email)]

The iframe shims test broke with your initial checkin:
http://build.webkit.org/results/GTK%20Linux%2032-bit%20Debug/r53075%20(2271)/plugins/iframe-shims-stderr.txt

[Gustavo Noronha (kov)]

(In reply to comment #15)
> The iframe shims test broke with your initial checkin:
> http://build.webkit.org/results/GTK%20Linux%2032-bit%20Debug/r53075%20(2271)/plugins/iframe-shims-stderr.txt

Yeah, it did look like. The revert did revert the relevant change, though, so I'm at a loss. Xan is getting us a clean rebuild in the debug bots to see if the assert goes away.

[Gustavo Noronha (kov)]

Comment on attachment 46232 [details]
do not call committedLoad on finishedLoading

Re-landed as r53138, after we figured the assert was not caused by this patch.

[Oliver Hunt]

Comment on attachment 46269 [details]
avoid emitting load status signals when loading error pages

r=me

[Gustavo Noronha (kov)]

Last patch landed as r53294.