[Webkit-unassigned] [Bug 29248] [Qt] [API] Make it possible to have 'invisible' loads

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=29248





--- Comment #5 from Kenneth Rohde Christiansen <kenneth.christiansen at openbossa.org>  2009-09-14 18:25:27 PDT ---
(In reply to comment #4)
> Created an attachment (id=39578)
 --> (https://bugs.webkit.org/attachment.cgi?id=39578) [details]
> patch v0.1.1 - basic invisible load impl 
> 
> cleaner patch

You ought to tell what you have changed, so we don't have to diff the patches.
Basically it doesn't seem to fix any of my concerns.

Looking at the patch I see

+/*
+   \since 4.6
+   Returns if a given url \a url is in the history.
+*/
+int QWebHistory::contains(const QUrl& url) cons

I guess this could/should be separate bug. Maybe it should even be called
something like findUrl, to indicate that it might be an expensive function
call.

+    void setEnabled(bool status);

A setEnabled method taking a bool called status? status is a very bad variable
name not indicating whether it will be enabled or disabled. Qt normally uses
'on'. You should always look at the Qt class libraries for creating similar
API.

+/*
+   \since 4.6
+   Returns if the history is in 'locked' state or not.
+   \sa setEnabled()
+*/
+bool QWebHistory::isEnabled() const

This comment leds to be think that the name "enabled" might not be a good fit.

Try not to make unnecessary changes:

-    m_loadError = ResourceError(); // clears the previous error
+    // clears the previous load data
+    m_loadError = ResourceError();


You made it possible to enable/disable history, but in the FrameLoaderClientQt
I found a line where you always enable it! regarding on what the current state
is. That seems like a bug:

@@ -376,7 +376,10 @@ void FrameLoaderClientQt::dispatchDidFinishLoad()
+ m_webFrame->d->page->history()->setEnabled(true);

Regarding

+++ b/WebCore/loader/FrameLoader.cpp
@@ -3379,7 +3379,7 @@ void FrameLoader::checkLoadCompleteForThisFrame()
-            if (shouldReset && item)
+            if (shouldReset && item && (item == m_currentHistoryItem))

This affects all ports, and it is not clear for me what effect it will have.
Maybe make it another bug?

This as well:

-    void setEnabled(bool);
+    void setEnabled(bool enabled, bool clear = true);

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.