[webkit-dev] Regression fallout from change to make Frames start with an empty document

2007-05-12 Thread Maciej Stachowiak


My change to make Frames start with an empty document instead of  
creating one on demand caused some regressions (mostly in areas of  
the code that are not under automated test). I still think it is a  
fundamentally sound change and I'll try to fix the open regressions  
ASAP (hopefully I can figure out how to make test cases for these as  
well):


REGRESSION (r21367): Local css ignored until page is refreshed
http://bugs.webkit.org/show_bug.cgi?id=13661

REGRESSION:  Load never completes for nonexistent unqualified domain  
names

http://bugs.webkit.org/show_bug.cgi?id=13683

REGRESSION: Assertion failure in  
WebCore::FrameLoader::restoreScrollPositionAndViewState() going back  
from fark.com Photoshop contest

http://bugs.webkit.org/show_bug.cgi?id=13684

REGRESSION: Crash when starting Webkit with JavaScript disabled
http://bugs.webkit.org/show_bug.cgi?id=13691

REGRESSION: Progress bar never completes on link click that downloads
http://bugs.webkit.org/show_bug.cgi?id=13694


This also broke the Qt and Gdk ports. I will help to fix those but I  
could use someone who can easily build and test these ports to help  
me out. I think there are two basic problems:


1) Frame::init() needs to be called every time a new Frame is  
allocated, after it is attached to the frame tree.


2) FrameLoaderClient::committedLoad must ensure it calls  
FrameLoader::setEncoding before passing the first data chunk to the  
document. I think this is actually a remaining architectural problem  
with the loader, we are counting on the client to do too much in this  
case to have things work properly - I'd like to unwind this logic at  
some point to not rely on the client so much. In the short run, just  
adding the call is likely to just work.



Sorry about the breakage. I think this change will still be good in  
the long run.


Regards,
Maciej

___
webkit-dev mailing list
webkit-dev@lists.webkit.org
http://lists.webkit.org/mailman/listinfo/webkit-dev


[webkit-dev] Fix for Qt build

2007-05-12 Thread Maciej Stachowiak


I can't check this in right now because the SVN server is temporarily  
out of disk, but this appears to mostly fix the Qt build, at least in  
QtLauncher. Rob Buis helped me test and is running layout tests now.


Index: WebKitQt/ChangeLog
===
--- WebKitQt/ChangeLog  (revision 21427)
+++ WebKitQt/ChangeLog  (working copy)
@@ -1,3 +1,14 @@
+2007-05-12  Maciej Stachowiak  [EMAIL PROTECTED]
+
+Reviewed by Rob Buis.
+
+- call Frame::init as needed - this prevents crashes but  
pages don't appear.

+
+* Api/qwebframe.cpp:
+(QWebFramePrivate::init):
+* WebKitPart/WebKitPart.cpp:
+(WebKitPart::initView):
+
2007-05-08  Steve Falkenburg  [EMAIL PROTECTED]
 Reviewed by Ada.
Index: WebKitQt/Api/qwebframe.cpp
===
--- WebKitQt/Api/qwebframe.cpp  (revision 21427)
+++ WebKitQt/Api/qwebframe.cpp  (working copy)
@@ -79,6 +79,7 @@
 frameView-setScrollArea(qframe);
 frameView-setAllowsScrolling(frameData-allowsScrolling);
 frame-setView(frameView.get());
+frame-init();
 eventHandler = frame-eventHandler();
}
Index: WebKitQt/WebKitPart/WebKitPart.cpp
===
--- WebKitQt/WebKitPart/WebKitPart.cpp  (revision 21427)
+++ WebKitQt/WebKitPart/WebKitPart.cpp  (working copy)
@@ -123,6 +123,8 @@
 m_frame-setView(frameView);
 m_frameView-setParentWidget(parentWidget);
+m_frame-init();
+
 // Initialize KParts widget...
 setWidget(m_frame-view()-qwidget());
}

___
webkit-dev mailing list
webkit-dev@lists.webkit.org
http://lists.webkit.org/mailman/listinfo/webkit-dev


Re: [webkit-dev] Fix for Qt build

2007-05-12 Thread Holger Freyther


Am 12.05.2007 um 12:32 schrieb Maciej Stachowiak:



I can't check this in right now because the SVN server is  
temporarily out of disk, but this appears to mostly fix the Qt  
build, at least in QtLauncher. Rob Buis helped me test and is  
running layout tests now.


Hey,
so the Gdk port remains and I'm a bit lost and won't have time before  
monday. At least the Frame::init is the obvious change but I see a  
crash on the google site. It is reproducable by starting the  
GdkLauncher and clicking on any link. What confuses me is that in the  
below backtrace there is no way that FrameLoaderClientGdk can call  
setEncoding. I don't know the relation between ResourceLoader,  
FrameLoader, MainResourceLoader, DocumentLoader and  
ResourceHandleManager yet so I don't know who should call into the  
FrameLoaderClient and when.


From yesterday's research I saw that platform//network/gdk is rather  
incomplete, so it might be partly to blame. What I see is that  
directly from curl callback didReceiveData gets called and gets  
delivered up to the FrameLoader without ever calling into  
FrameLoaderClient or I forgot to set a breakpoint and missed a  
position where I can/should call setEncoding from.


What I will look into on Monday is:
-Where and when is the document of the Frame gets destroyed
	-What is the platform/network code omitting. I know of not handling  
HTTP response headers at all.
	-See if any unimplemented method of the FrameLoaderClientGdk gets  
called (Currently it looks like   
WebCore::FrameLoaderClientGdk::canCachePage() is the only one)




--- a/WebKitTools/GdkLauncher/main.cpp
+++ b/WebKitTools/GdkLauncher/main.cpp
@@ -213,6 +213,7 @@ int main(int argc, char* argv[])
 gFrame-setView(frameView);
 frameView-ScrollView::setDrawable(frameWindow-window);
+gFrame-init();
 gFrame-loader()-load(ResourceRequest(url));
 gtk_main();
#if 0 // FIXME: this crashes at the moment. needs to provide DragClient





0xb7a6d598 in WebCore::FrameLoader::saveDocumentState  
(this=0x8199630) at ../../../WebCore/loader/FrameLoader.cpp:3755

3755ASSERT(document);
(gdb) bt
#0  0xb7a6d598 in WebCore::FrameLoader::saveDocumentState  
(this=0x8199630) at ../../../WebCore/loader/FrameLoader.cpp:3755
#1  0xb7a7d283 in WebCore::FrameLoader::closeURL (this=0x8199630)  
at ../../../WebCore/loader/FrameLoader.cpp:627
#2  0xb7a7dea8 in WebCore::FrameLoader::transitionToCommitted  
(this=0x8199630, [EMAIL PROTECTED]) at ../../../WebCore/loader/ 
FrameLoader.cpp:2401
#3  0xb7a7ea50 in WebCore::FrameLoader::commitProvisionalLoad  
(this=0x8199630, [EMAIL PROTECTED]) at ../../../WebCore/ 
loader/FrameLoader.cpp:2359
#4  0xb7a64201 in WebCore::DocumentLoader::commitIfReady  
(this=0x8236e90) at ../../../WebCore/loader/DocumentLoader.cpp:305

#5  0xb7a64c6e in WebCore::DocumentLoader::commitLoad (this=0x8236e90,
data=0x824510b htmlheadmeta http-equiv=\content-type\  
content=\text/html; charset=ISO-8859-1\titleGoogle/ 
titlestyle!--\nbody,td,a,p,.h{font-family:arial,sans-serif}\n.h 
{font-size:20px}\n.h{color:#3366cc}\n, length=1255) at ../../../ 
WebCore/loader/DocumentLoader.cpp:345

#6  0xb7a64d2e in WebCore::DocumentLoader::receivedData (this=0x8236e90,
data=0x824510b htmlheadmeta http-equiv=\content-type\  
content=\text/html; charset=ISO-8859-1\titleGoogle/ 
titlestyle!--\nbody,td,a,p,.h{font-family:arial,sans-serif}\n.h 
{font-size:20px}\n.h{color:#3366cc}\n, length=1255) at ../../../ 
WebCore/loader/DocumentLoader.cpp:359

#7  0xb7a73117 in WebCore::FrameLoader::receivedData (this=0x8199630,
data=0x824510b htmlheadmeta http-equiv=\content-type\  
content=\text/html; charset=ISO-8859-1\titleGoogle/ 
titlestyle!--\nbody,td,a,p,.h{font-family:arial,sans-serif}\n.h 
{font-size:20px}\n.h{color:#3366cc}\n, length=1255) at ../../../ 
WebCore/loader/FrameLoader.cpp:2052

#8  0xb7a9ac24 in WebCore::MainResourceLoader::addData (this=0x822e9d0,
data=0x824510b htmlheadmeta http-equiv=\content-type\  
content=\text/html; charset=ISO-8859-1\titleGoogle/ 
titlestyle!--\nbody,td,a,p,.h{font-family:arial,sans-serif}\n.h 
{font-size:20px}\n.h{color:#3366cc}\n, length=1255,  
allAtOnce=false) at ../../../WebCore/loader/MainResourceLoader.cpp:136
#9  0xb7aa11d7 in WebCore::ResourceLoader::didReceiveData  
(this=0x822e9d0,
data=0x824510b htmlheadmeta http-equiv=\content-type\  
content=\text/html; charset=ISO-8859-1\titleGoogle/ 
titlestyle!--\nbody,td,a,p,.h{font-family:arial,sans-serif}\n.h 
{font-size:20px}\n.h{color:#3366cc}\n, length=1255,  
lengthReceived=0, allAtOnce=false) at ../../../WebCore/loader/ 
ResourceLoader.cpp:208
#10 0xb7a9a99e in WebCore::MainResourceLoader::didReceiveData  
(this=0x822e9d0,
data=0x824510b htmlheadmeta http-equiv=\content-type\  
content=\text/html; charset=ISO-8859-1\titleGoogle/ 
titlestyle!--\nbody,td,a,p,.h{font-family:arial,sans-serif}\n.h 
{font-size:20px}\n.h{color:#3366cc}\n, length=1255,  
lengthReceived=0, 

Re: [webkit-dev] Fix for Qt build

2007-05-12 Thread Holger Freyther


Am 12.05.2007 um 13:28 schrieb Holger Freyther:



Am 12.05.2007 um 12:32 schrieb Maciej Stachowiak:



I can't check this in right now because the SVN server is  
temporarily out of disk, but this appears to mostly fix the Qt  
build, at least in QtLauncher. Rob Buis helped me test and is  
running layout tests now.




Okay,
the difference between Mac and Gdk is in  
FrameLoaderClient::canCachePage. If canCachePage returns false, clear 
() will be called which sets m_frame-d-m_doc-m_ptr to 0. And on  
load setEncoding comes after calling saveDocumentState. So there is  
no way to restore/has a valid document.


I don't know how to fix it, but at least this explains why it crashes  
on Gdk and not on Mac. I think we should review codepath's ending in  
a clear().




void FrameLoader::provisionalLoadStarted()
{
m_firstLayoutDone = false;
cancelRedirection(true);
m_client-provisionalLoadStarted();

if (canCachePage()) {
if (m_client-canCachePage()) {
if (!m_currentHistoryItem-cachedPage()) {
cachePageToHistoryItem(m_currentHistoryItem.get());
purgePageCache();
}
} else {
// Put the document into a null state, so it can be  
restored correctly.·

clear();
}
}
}

PS: Now I can finally return to my text 
book___
webkit-dev mailing list
webkit-dev@lists.webkit.org
http://lists.webkit.org/mailman/listinfo/webkit-dev


Re: [webkit-dev] Regression fallout from change to make Frames start with an empty document

2007-05-12 Thread Geoffrey Garen
I believe we should add this one to the list: rdar://problem/ 
5198885 Crash or ASSERT below FrameLoader::commitProvisionalLoad  
when navigating away from bookmarks view


Geoff

On May 12, 2007, at 1:14 AM, Maciej Stachowiak wrote:



My change to make Frames start with an empty document instead of  
creating one on demand caused some regressions (mostly in areas of  
the code that are not under automated test). I still think it is a  
fundamentally sound change and I'll try to fix the open regressions  
ASAP (hopefully I can figure out how to make test cases for these  
as well):


REGRESSION (r21367): Local css ignored until page is refreshed
http://bugs.webkit.org/show_bug.cgi?id=13661

REGRESSION:  Load never completes for nonexistent unqualified  
domain names

http://bugs.webkit.org/show_bug.cgi?id=13683

REGRESSION: Assertion failure in  
WebCore::FrameLoader::restoreScrollPositionAndViewState() going  
back from fark.com Photoshop contest

http://bugs.webkit.org/show_bug.cgi?id=13684

REGRESSION: Crash when starting Webkit with JavaScript disabled
http://bugs.webkit.org/show_bug.cgi?id=13691

REGRESSION: Progress bar never completes on link click that downloads
http://bugs.webkit.org/show_bug.cgi?id=13694


This also broke the Qt and Gdk ports. I will help to fix those but  
I could use someone who can easily build and test these ports to  
help me out. I think there are two basic problems:


1) Frame::init() needs to be called every time a new Frame is  
allocated, after it is attached to the frame tree.


2) FrameLoaderClient::committedLoad must ensure it calls  
FrameLoader::setEncoding before passing the first data chunk to the  
document. I think this is actually a remaining architectural  
problem with the loader, we are counting on the client to do too  
much in this case to have things work properly - I'd like to unwind  
this logic at some point to not rely on the client so much. In the  
short run, just adding the call is likely to just work.



Sorry about the breakage. I think this change will still be good in  
the long run.


Regards,
Maciej

___
webkit-dev mailing list
webkit-dev@lists.webkit.org
http://lists.webkit.org/mailman/listinfo/webkit-dev


___
webkit-dev mailing list
webkit-dev@lists.webkit.org
http://lists.webkit.org/mailman/listinfo/webkit-dev


Re: [webkit-dev] Fix for Qt build

2007-05-12 Thread Maciej Stachowiak


On May 12, 2007, at 7:59 AM, Holger Freyther wrote:



Am 12.05.2007 um 13:28 schrieb Holger Freyther:



Am 12.05.2007 um 12:32 schrieb Maciej Stachowiak:



I can't check this in right now because the SVN server is  
temporarily out of disk, but this appears to mostly fix the Qt  
build, at least in QtLauncher. Rob Buis helped me test and is  
running layout tests now.




Okay,
the difference between Mac and Gdk is in  
FrameLoaderClient::canCachePage. If canCachePage returns false,  
clear() will be called which sets m_frame-d-m_doc-m_ptr to 0.  
And on load setEncoding comes after calling saveDocumentState. So  
there is no way to restore/has a valid document.


Oh - that was actually a bug and I believe I've landed a fix.


I don't know how to fix it, but at least this explains why it  
crashes on Gdk and not on Mac. I think we should review codepath's  
ending in a clear().


void FrameLoader::provisionalLoadStarted()
{
m_firstLayoutDone = false;
cancelRedirection(true);
m_client-provisionalLoadStarted();

if (canCachePage()) {
if (m_client-canCachePage()) {
if (!m_currentHistoryItem-cachedPage()) {
cachePageToHistoryItem(m_currentHistoryItem.get());
purgePageCache();
}
} else {
// Put the document into a null state, so it can be  
restored correctly.·

clear();
}
}
}


The code now reads like this:

if (canCachePage()  m_client-canCachePage()  ! 
m_currentHistoryItem-cachedPage()) {

cachePageToHistoryItem(m_currentHistoryItem.get());
purgePageCache();
}


___
webkit-dev mailing list
webkit-dev@lists.webkit.org
http://lists.webkit.org/mailman/listinfo/webkit-dev


[webkit-dev] Patch for notImplemented() macros

2007-05-12 Thread Kevin Ollivier

Hi Darin and all,

On Apr 9, 2007, at 3:12 PM, Darin Adler wrote:


On Apr 8, 2007, at 3:38 PM, Kevin Ollivier wrote:

One other issue I wanted to bring up that I've run into is an  
issue with needing the notImplementedGdk() macro to be defined  
even though we're not using the GDK port, as we use the CURL  
downloader. We've hacked around it in our sources for now, but  
IMHO we'll have to either have both notImplemented() and  
notImplementedGdk() available to the GDK port, or remove the need  
for a separate notImplementedGdk() macro if that's possible.


Also, there's a lot of duplication of notImplemented() in various  
sources anyway, so I thought it might be worth bringing up the  
idea of consolidating all those definitions into a header  
somewhere (Assertions.h?). As it seems different ports might want  
to implement it different ways, we can use #If PLATFORM(XYZ)  
blocks to allow alternative definitions.


What do you guys think? Do you see doing it this way causing any  
problems for any of the ports? If you agree, any suggestions on  
where notImplemented() should go?


I think we should add WebCore/platform/NotImplemented.h and use the  
#if PLATFORM(XYZ) approach.


Well, it's taken me a little while, but I've finally submitted this  
as a patch in Bugzilla. The bug report is here:


http://bugs.webkit.org/show_bug.cgi?id=13699

Since this mostly affects the GDK and Qt ports, which I'm not setup  
to build, I'd appreciate it someone setup for those ports could give  
this patch a try.


Regards,

Kevin


-- Darin



___
webkit-dev mailing list
webkit-dev@lists.webkit.org
http://lists.webkit.org/mailman/listinfo/webkit-dev