Bug 33428 - [GTK] couple fixes for signal emissions, and property notifications
Summary: [GTK] couple fixes for signal emissions, and property notifications
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Page Loading (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Gustavo Noronha (kov)
URL:
Keywords: Gtk
Depends on:
Blocks:
 
Reported: 2010-01-09 13:23 PST by Gustavo Noronha (kov)
Modified: 2010-01-14 15:09 PST (History)
7 users (show)

See Also:


Attachments
do not call committedLoad on finishedLoading (2.72 KB, patch)
2010-01-09 13:30 PST, Gustavo Noronha (kov)
gns: commit-queue-
Details | Formatted Diff | Diff
avoid emitting load signals for error pages loads (7.32 KB, patch)
2010-01-09 13:36 PST, Gustavo Noronha (kov)
gns: commit-queue-
Details | Formatted Diff | Diff
do not call committedLoad on finishedLoading (3.15 KB, patch)
2010-01-09 13:41 PST, Gustavo Noronha (kov)
gns: commit-queue-
Details | Formatted Diff | Diff
do not call committedLoad on finishedLoading (2.72 KB, patch)
2010-01-10 05:08 PST, Gustavo Noronha (kov)
no flags Details | Formatted Diff | Diff
avoid emitting load status signals when loading error pages (6.89 KB, patch)
2010-01-11 04:26 PST, Gustavo Noronha (kov)
oliver: review+
gns: commit-queue-
Details | Formatted Diff | Diff
epiphany patch to remove workarounds (5.52 KB, patch)
2010-01-11 04:27 PST, Gustavo Noronha (kov)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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.