Summary: | [GTK] couple fixes for signal emissions, and property notifications | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Gustavo Noronha (kov) <gustavo> | ||||||||||||||
Component: | Page Loading | Assignee: | 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
Gustavo Noronha (kov)
2010-01-09 13:23:19 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.
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
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.
Created attachment 46217 [details]
do not call committedLoad on finishedLoading
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
Why does this patch intentionally move the *? - FrameLoader* frameLoader = loader->frameLoader(); + FrameLoader *frameLoader = loader->frameLoader(); That seems like a violation of the style guide... (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 =); Created attachment 46232 [details]
do not call committedLoad on finishedLoading
Hopefully this one has not style issues.
Comment on attachment 46232 [details]
do not call committedLoad on finishedLoading
OK.
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.
Created attachment 46271 [details]
epiphany patch to remove workarounds
This is what we can do in Ephy.
Comment on attachment 46232 [details] do not call committedLoad on finishedLoading Landed as r53075. 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. 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 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 (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 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 on attachment 46269 [details]
avoid emitting load status signals when loading error pages
r=me
Last patch landed as r53294. |