Bug 33428

Summary: [GTK] couple fixes for signal emissions, and property notifications
Product: WebKit Reporter: Gustavo Noronha (kov) <gustavo>
Component: Page LoadingAssignee: Gustavo Noronha (kov) <gustavo>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, christian, eric, gustavo, tevaum, webkit.review.bot, xan.lopez
Priority: P2 Keywords: Gtk
Version: 528+ (Nightly build)   
Hardware: PC   
OS: OS X 10.5   
Attachments:
Description Flags
do not call committedLoad on finishedLoading
gustavo: commit-queue-
avoid emitting load signals for error pages loads
gustavo: commit-queue-
do not call committedLoad on finishedLoading
gustavo: commit-queue-
do not call committedLoad on finishedLoading
none
avoid emitting load status signals when loading error pages
oliver: review+, gustavo: commit-queue-
epiphany patch to remove workarounds none

Description Gustavo Noronha (kov) 2010-01-09 13:23:19 PST
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.
Comment 1 Gustavo Noronha (kov) 2010-01-09 13:30:08 PST
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.
Comment 2 WebKit Review Bot 2010-01-09 13:35:11 PST
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
Comment 3 Gustavo Noronha (kov) 2010-01-09 13:36:19 PST
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.
Comment 4 Gustavo Noronha (kov) 2010-01-09 13:41:47 PST
Created attachment 46217 [details]
do not call committedLoad on finishedLoading
Comment 5 WebKit Review Bot 2010-01-09 13:46:03 PST
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
Comment 6 Adam Barth 2010-01-09 16:36:12 PST
Why does this patch intentionally move the *?

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

That seems like a violation of the style guide...
Comment 7 Gustavo Noronha (kov) 2010-01-10 05:04:25 PST
(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 =);
Comment 8 Gustavo Noronha (kov) 2010-01-10 05:08:29 PST
Created attachment 46232 [details]
do not call committedLoad on finishedLoading

Hopefully this one has not style issues.
Comment 9 Xan Lopez 2010-01-11 03:25:05 PST
Comment on attachment 46232 [details]
do not call committedLoad on finishedLoading

OK.
Comment 10 Gustavo Noronha (kov) 2010-01-11 04:26:55 PST
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.
Comment 11 Gustavo Noronha (kov) 2010-01-11 04:27:53 PST
Created attachment 46271 [details]
epiphany patch to remove workarounds

This is what we can do in Ephy.
Comment 12 Gustavo Noronha (kov) 2010-01-11 07:16:11 PST
Comment on attachment 46232 [details]
do not call committedLoad on finishedLoading

Landed as r53075.
Comment 13 Gustavo Noronha (kov) 2010-01-11 09:57:51 PST
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.
Comment 14 Eric Seidel (no email) 2010-01-11 12:10:00 PST
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
Comment 15 Eric Seidel (no email) 2010-01-11 12:10:47 PST
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
Comment 16 Gustavo Noronha (kov) 2010-01-11 12:42:31 PST
(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.
Comment 17 Gustavo Noronha (kov) 2010-01-12 05:49:09 PST
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.
Comment 18 Oliver Hunt 2010-01-13 22:45:07 PST
Comment on attachment 46269 [details]
avoid emitting load status signals when loading error pages

r=me
Comment 19 Gustavo Noronha (kov) 2010-01-14 15:09:32 PST
Last patch landed as r53294.