Bug 29248 – [Qt] [API] Make it possible to have 'invisible' loads

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Qt

[Antonio Gomes]

For substitution loads (e.g. setHtml, setContent and friends) it could be a nice add to the API to make it possible to have what we've calling 'invisible' loads. Feature basically consists in loading a given HTML content (for now lets consider it a QString):

1) w/o necessarely changing WebCore's backforward and global history content.
2) w/o getting loading signals emitted

 ==========
* Use case *
 ==========

1) user enter a valid url (http://www.google.com) then enters any non-existent url (e.g. http://xxx.yyy).
   The client browser might want to feedback the user about the loading error, maybe by calling setHtml() passing in
   some loading error template page.
   The problem is that currently setHtml starts a completely new load. so in our backforward history content one would end up w/
   -> google.com -> xxx.yyy -> baseUrl passed in to setHtml, if any.

   Desired backforward content is:
   -> google.com -> xxx.yyy (the error page shown). So calling RELOAD, back or forward would work as user expect.

 ==================
* Proposed solution *
 ==================

A working solution  (not ready yet for a final review) I would like to request comments for is attached (patch v0.1). It basically overloads the existenting QWebFrame::setHtml method, extending the way it works if some parameters are passed in. If the approach is good, it could get into setContent() method too in the same way.

Some points are still to be properly defined, for example: About (2), I am sure if not emitting all load signals is good, or just 'urlChanged' (as currently in the patch).

please comments are welcome.

ps: not autotests yet, but existent tests passed.
ps: no  difference in layout-test results after the changes in webcore.

[Antonio Gomes]

Created attachment 39556 [details]
patch v0.1 - basic  invisible load impl

first implementation draft

[Kenneth Rohde Christiansen]

So basically I didn't like this API very much. 

It is hard to use and provides more power than I think the users need.

Your MainFrameLoadMode (ugly name) is a flag, but that is not obvious.

Actually I would prefer a LoadMode enum (NormalLoad, SubstitutionLoad) and the substitution load would do the following

exclude global/session history and not modify the internal url, thus still making url() and requestedUrl() return the previous url. Thus the substitution is transparent.

Maybe this is hard to implement and because of this you decided to block some events, but at least this doesn't have to be exposed in the API.

[Kenneth Rohde Christiansen]

+    void setHtml(const QString &html, const QUrl &baseUrl, const QUrl &substituteUrl, LoadMode mode);

setHtml taking a substitution url? That just seems wrong, a load() method should be used for this, thus implement a load(..) that takes a LoadMode as well.

[Antonio Gomes]

Created attachment 39578 [details]
patch v0.1.1 - basic invisible load impl 

cleaner patch

[Kenneth Rohde Christiansen]

(In reply to comment #4)
> Created an attachment (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);

[Kenneth Rohde Christiansen]

To me

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

seems a bit backward. So the WebCore setEnabled always clears history.

So maybe it would be better to separate our the clear code of setEnabled to a clear() method (many webcore classes have clear() members)

and make setEnabled() call clear() and enable the history. 

If very little code is using the setEnabled method, you can change that to call clear() and then setEnabled(true)

If that is not the case, I would suggest that you make another method called setLocked() or so, and make setEnabled(true) call clear() + setLocked(false). This might be less desirable though.

[Kenneth Rohde Christiansen]

> ps: no  difference in layout-test results after the changes in webcore.

Running with Qt or mac? We [Qt] only checks like 50% of the tests available.

[Kenneth Rohde Christiansen]

(In reply to comment #7)
> > ps: no  difference in layout-test results after the changes in webcore.
> 
> Running with Qt or mac? We [Qt] only checks like 50% of the tests available.

I know you don't have a mac :-), so what I'm trying to say is that if you were making a Qt specific change (let's say to FontQt.cpp) then this would be a sufficient test for us, but this is a cross platform change.

If it breaks mac or so, it will just get reverted. Keep that in mind. This is really the reason why I would like you to commit the WebCore required changes first in a separate patch.

[Kenneth Rohde Christiansen]

Eric, do you know what Chrome does for showing error pages etc and not touch the session/global history? 

I'm wondering why we need WebCore changes for this, when Google Chome and Safari obviously don't.

[Eric Seidel (no email)]

Bradey or Darin Fisher would know.

[Brady Eidson]

This is what SubstituteData is fore

[Brady Eidson]

*sigh*
s/fore/for/

[Antonio Gomes]

Created attachment 39629 [details]
patch 0.2 - same functionality, but no touch to WebCore or qwebhistory

[Antonio Gomes]

thanks for the comments.

this patch v0.2 does not make changes in WebCore and qwebhistory any more, but keeps the same functionality, so it might easier to understand

(In reply to comment #2)
> Your MainFrameLoadMode (ugly name) is a flag, but that is not obvious.
> 
> Actually I would prefer a LoadMode enum (NormalLoad, SubstitutionLoad) and the
> substitution load would do the following

Done. actually I did it as following:

* NormalLoad.
* SubstitutionLoad -> no back/forward or global history change.
use case: a custom home page, that after user navigates ways, it is not desired it to leave any footprint.
* NoGlobalHistoryChange -> no global history change for the load.
use case: a ErrorPage set

maybe we do not need the later flag: we could make 'substitutionLoad' not to block session history but the global, but instead add API for locking the session history as in the previous patches (another bug).

> setHtml taking a substitution url? That just seems wrong, a load() method
> should be used for this, thus implement a load(..) that takes a LoadMode as
> well.

/me thinks about it ...

> exclude global/session history and not modify the internal url, thus still
> making url() and requestedUrl() return the previous url. Thus the substitution is transparent.

i am also tending to leave the substituteUrl parameter out (but kept it for this patch version)

(In reply to comment #11)
> This is what SubstituteData is fore

thanks. I was making use of it. but realized that no passing failingUrl to SubstituteData() make history not get changed.

[Kenneth Rohde Christiansen]

Mid-air collision, reposting!

So what about the concerns that I have raised? like setHtml taking an url etc.

Are you going to solve these? If you disagree (or are uncertain) with any of
them, let's have the discussion here so that everyone can join.

[Antonio Gomes]

Created attachment 39630 [details]
patch 0.2.1 - same functionality as 0.2, but right changelog

[Kenneth Rohde Christiansen]

> Done. actually I did it as following:
> 
> * NormalLoad.
> * SubstitutionLoad -> no back/forward or global history change.
> use case: a custom home page, that after user navigates ways, it is not desired
> it to leave any footprint.
> * NoGlobalHistoryChange -> no global history change for the load.
> use case: a ErrorPage set

Ah, I like that more.

> maybe we do not need the later flag: we could make 'substitutionLoad' not to
> block session history but the global, but instead add API for locking the
> session history as in the previous patches (another bug).

Yes, I dont think the latter is needed, thought I didn't really understand what
you said above. Can you explain the differences between the two usecases a bit
more clear, please?

> > setHtml taking a substitution url? That just seems wrong, a load() method
> > should be used for this, thus implement a load(..) that takes a LoadMode as
> > well.
> 
> /me thinks about it ...

load methods takes urls, setHtml methods takes "data". So let's not mix those
two :-)


> i am also tending to leave the substituteUrl parameter out (but kept it for
> this patch version)

Please do.

> (In reply to comment #11)
> > This is what SubstituteData is for
> 
> thanks. I was making use of it. but realized that no passing failingUrl to
> SubstituteData() make history not get changed.

Can you enlighten me on, why it changes the history (session? global?) when you
give it a failingUrl? What is that failing url for? showing error pages, maybe?
:-)

[Kenneth Rohde Christiansen]

+    if (mode == StandardLoad)
+        return setHtml(html, baseUrl);

Don't you think you should modify the original setHtml to call this one? now that you cannot modify it due to binary compatibility. It just seems more right to me.

[Kenneth Rohde Christiansen]

Another thing that I stumbled on.

+    enum LoadMode {
+        NoGlobalHistoryChange = 0x2,
+        SubstitutionLoad = 0x4,
+        StandardLoad = 0x8
+    };

Why the 0x2 etc? This isn't supposed to be a flag right? If so I would also have preferred it to be called LoadModeFlag or so.

[Kenneth Rohde Christiansen]

Maybe a stupid question, but why does the

void setHtml(const QString &html, const QUrl &baseUrl = QUrl());

function modify the session and global history? and emit urlChanged(). It really does that? or is that a bug?

I still like the idea of specifying a load type when calling load(...)

[Antonio Gomes]

> load methods takes urls, setHtml methods takes "data". So let's not mix those
> two :-)

in my mind, that is what substituteUrl would stands for here.

case_1)

-> suppose we want to load a "about:plugins" url via setHtml. how to do it ?

since current setHtml implementation does not let one to pass the Url being loaded (but only data and baseUrl), WebCore would never know that "about:plugins" is being loaded. What people do is to abuse the meaning of 'baseUrl', and pass the Url to be referred internally by WebCore as the baseUrl. so w/ current API we would do this:

frame->setHtml("<html> about:plugin data</hmtl>", "about:plugins");

in the case the html data has relative paths to be resolved, then the "baseUrl" meaning can not be "abused" and will have to be the the real "baseUrl". method call would change to

frame->setHtml("<html> about:plugin data</hmtl>", "/path/to/data");

case_2) looking at arora's code we see (it is setting a html error page):

(...)
    html = tr("any html code here");
    notFoundFrame->setHtml(html, replyUrl);
(...)

it also does not use baseUrl as baseUrl. Instead it passes the "replyUrl" it is willing to set the page error for, so WebCore can refer to it. That way, RELOADs will try to load "replyUrl". But what is the html data string had relative paths to be resolved ?

imho, it might be a dangerious thing.

w/ the substitutionUrl parameter, it'd be

(...)
    notFoundFrame->setHtml(html, baseUrl, replyUrl);
(...)

> Can you enlighten me on, why it changes the history (session? global?) when you
> give it a failingUrl? What is that failing url for? showing error pages, maybe?
> :-)

so when a valid SubstituteData is passed to FrameLoader::load(), a 'StandardFrameLoadType' is happenning (history is touched and blablabla). However, while the documentLoader is getting committed, it calls FrameLoader::updateHistoryForStandardLoad(). In the method body there is a check 

    const KURL& historyURL = documentLoader()->urlForHistory();
        if (!historyURL.isEmpty()) {
            addBackForwardItemClippedAtTarget(true); <--- session history
            m_client->updateGlobalHistory();         <--- global history
 
for valid substituteData, urlForHistory calls unreachableURL() that called substitute.failingUrl . so if we pass no failingUrl to substitute data, there is no URL to be added to history.

was it clear ?

> +    enum LoadMode {
> +        NoGlobalHistoryChange = 0x2,
> +        SubstitutionLoad = 0x4,
> +        StandardLoad = 0x8
> +    };
> 
> Why the 0x2 etc? This isn't supposed to be a flag right? If so I would also
> have preferred it to be called LoadModeFlag or so.

they are not flags but simple enum's now.

> void setHtml(const QString &html, const QUrl &baseUrl = QUrl());
> 
> function modify the session and global history? and emit urlChanged(). It
> really does that? or is that a bug?

yes ; yes ; yes ; not sure if it is a bug. maybe a LACK in WebCore.

[Kenneth Rohde Christiansen]

(In reply to comment #21)
> > load methods takes urls, setHtml methods takes "data". So let's not mix those
> > two :-)
> 
> in my mind, that is what substituteUrl would stands for here.

I disagree... more comments below


> frame->setHtml("<html> about:plugin data</hmtl>", "about:plugins");
> (...)
>     html = tr("any html code here");
>     notFoundFrame->setHtml(html, replyUrl);
> (...)

To me the above is abusing the API like using a trick. And we could provide a better API for this and not make our current one even worse

this could clearly be

html = "some html error message";
frame->setHtml(html, QUrl(), SubstitutionLoad);

But as I said before, to me it seems like a bug that setHtml changes the url/history.

> w/ the substitutionUrl parameter, it'd be
> 
> (...)
>     notFoundFrame->setHtml(html, baseUrl, replyUrl);
> (...)

notFoundFrame->setHtml(html, baseUrl, SubstitutionLoad) should do.

Actually when looking at the method, "Load" seems wrong. Maybe we should use Data instead

SubstituteData, NormalData ...

This even seems very similar to what WebCore apted for.

> so when a valid SubstituteData is passed to FrameLoader::load(), a
> 'StandardFrameLoadType' is happenning (history is touched and blablabla).
> However, while the documentLoader is getting committed, it calls
> FrameLoader::updateHistoryForStandardLoad(). In the method body there is a
> check 
> 
>     const KURL& historyURL = documentLoader()->urlForHistory();
>         if (!historyURL.isEmpty()) {
>             addBackForwardItemClippedAtTarget(true); <--- session history
>             m_client->updateGlobalHistory();         <--- global history
> 
> for valid substituteData, urlForHistory calls unreachableURL() that called
> substitute.failingUrl . so if we pass no failingUrl to substitute data, there
> is no URL to be added to history.
> 
> was it clear ?

So the failingUrl is more or less like your substituteUrl.

> they are not flags but simple enum's now.

Then please remove the = 0x2 etc.

> > void setHtml(const QString &html, const QUrl &baseUrl = QUrl());
> > 
> > function modify the session and global history? and emit urlChanged(). It
> > really does that? or is that a bug?
> 
> yes ; yes ; yes ; not sure if it is a bug. maybe a LACK in WebCore.

It depends on the behaviour we expect from the method. Personally I believe that the method should not touch session or global history, though there might be situations you want to to clear the internally url.

If that behaviour could be fixed, I would be happy, and I think that will serve for many of our situations.

Then we could try to make load(...) more powerful by allowing to specify LoadType and maybe a failingUrl.

[Simon Hausmann]

Comment on attachment 39630 [details]
patch 0.2.1 - same functionality as 0.2, but right changelog

I'm going to say r-, but I agree with the concept and the need to have this functionality.

First of all this patch is lacking an autotest. For changes in the loader, etc. we really need an automated test for this.

I also admit that I'm not happy about the API. setHtml becomes inconsistent to setContent and with four arguments
it becomes quite ugly.

If we decide not to change setHtml's behaviour, then I wonder if it would be easier to have a dedicated function for
the job needed, instead of trying to merge this into the setHtml function.

Does the error message that you'd like to display as substitute need to be generated dynamically? Could it be a property
on the QWebPage?

Would it make sense to have an extension in QWebPage that allows the application to provide substitute content
in case loading of a url fails?

[Kenneth Rohde Christiansen]

> I also admit that I'm not happy about the API. setHtml becomes inconsistent to
> setContent and with four arguments
> it becomes quite ugly.

as I said in my comments, I believe that the behaviour of that function is wrong. Why does it make sense for it to insert the baseUrl in the history? That just doesnt seem right to me.

As far as I understood it, the base url, is for relative urls inside the html provided.

> Does the error message that you'd like to display as substitute need to be
> generated dynamically? Could it be a property
> on the QWebPage?

Most of the time dynamically, yes. On the other hand, this is not just for error pages. It can be used for webcome pages etc (on open new tab etc)

> Would it make sense to have an extension in QWebPage that allows the
> application to provide substitute content
> in case loading of a url fails?

I don't think that is the best idea, as that would be a restriction and not work for special pages such as welcome pages.

[Simon Hausmann]

(In reply to comment #24)
> > I also admit that I'm not happy about the API. setHtml becomes inconsistent to
> > setContent and with four arguments
> > it becomes quite ugly.
> 
> as I said in my comments, I believe that the behaviour of that function is
> wrong. Why does it make sense for it to insert the baseUrl in the history? That
> just doesnt seem right to me.
> 
> As far as I understood it, the base url, is for relative urls inside the html
> provided.
> 
> > Does the error message that you'd like to display as substitute need to be
> > generated dynamically? Could it be a property
> > on the QWebPage?
> 
> Most of the time dynamically, yes. On the other hand, this is not just for
> error pages. It can be used for webcome pages etc (on open new tab etc)

Ok, that is another good use-case :)

Alternate idea:

If all we want to be able to is to disable the history, is it a state we can have on QWebPAge perhaps? (setHistoryEnabled(...))

[Antonio Gomes]

simon, thanks for looking at it. i have not requested for review yet because i knew these problems w/ the current approach.

> As far as I understood it, the base url, is for relative urls inside the html
> provided.

exactly. as i ponted out previously, arora for example makes use of this fragilily in the API:

setHtml("any string error page w/o external resources", requestedUrl) ... 

it  passes the requestedUrl as baseUrl just because the baseUrl goes to history and not because it is needed to resolved relative paths

i also agree that sethtml and setcontents need to have the same (similar) method signature/behaviour.

simon, in one of the obsolete patches i've added the setEnable method you mentioned about ... although it also required some changes in WebCore/history/BackForwardList.cpp since calling setEnabled(false) method cleans the list.


also, after studying and understanding webcore/loader/frameloader.cpp behaviour, i know how the right approach is now. i will detail it here in another comment or post to the -dev mailing list (so maybe brady can give a opnion here)

[Kenneth Rohde Christiansen]

The setHtml documentation has no info on it changing the url and touching the
history

KURL kurl(baseUrl);
WebCore::SubstituteData substituteData(data, WebCore::String("text/html"),
WebCore::String("utf-8"), kurl);

The whole problem here is the kurl, as it is being called as failing url

The problem is that documentLoader()->urlForHistory(); calls unreachableURL()
which returns the failingUrl (according to Antonio)


thus we run into this
    const KURL& historyURL = documentLoader()->urlForHistory();
        if (!historyURL.isEmpty()) {
            addBackForwardItemClippedAtTarget(true); <--- session history
            m_client->updateGlobalHistory();         <--- global history

WebCore::SubstituteData substituteData(data, WebCore::String("text/html"),
WebCore::String("utf-8"), KURL()); would fix it.

It seems that currently webcore make it possible for you to add an url to the
history for the substitute data. For instance I want to show a page but add
"about:plugins" to the history. This could be acomplished by adding another
setHtml function with another url argument

void QWebFrame::setHtml(const QString &html, const QUrl &baseUrl, const QUrl
&historyUrl)

Example:

setHtml("<html><body><img src="kenneth/image.png"></body></html>",
QUrl("file:///home"));

This will show a page with an image /home/kenneth/image.png and it will add
file:///home to the history

That seems like unintended behaviour


On the other hand I might want the ability to do setHtml("<html><body><img
src="kenneth/image.png"></body></html>", QUrl("file:///home"),
QUrl("about:arora"));

So it will add about:arora to the history and forward/back history

[Antonio Gomes]

exactly, good points.

the most important at the moment is to refine the API and decide about the desired behaviour

... and i can adapt the implement for it.

(In reply to comment #27)
> The setHtml documentation has no info on it changing the url and touching the
> history
> 
> KURL kurl(baseUrl);
> WebCore::SubstituteData substituteData(data, WebCore::String("text/html"),
> WebCore::String("utf-8"), kurl);
> 
> The whole problem here is the kurl, as it is being called as failing url
> 
> The problem is that documentLoader()->urlForHistory(); calls unreachableURL()
> which returns the failingUrl (according to Antonio)
> 
> 
> thus we run into this
>     const KURL& historyURL = documentLoader()->urlForHistory();
>         if (!historyURL.isEmpty()) {
>             addBackForwardItemClippedAtTarget(true); <--- session history
>             m_client->updateGlobalHistory();         <--- global history
> 
> WebCore::SubstituteData substituteData(data, WebCore::String("text/html"),
> WebCore::String("utf-8"), KURL()); would fix it.
> 
> It seems that currently webcore make it possible for you to add an url to the
> history for the substitute data. For instance I want to show a page but add
> "about:plugins" to the history. This could be acomplished by adding another
> setHtml function with another url argument
> 
> void QWebFrame::setHtml(const QString &html, const QUrl &baseUrl, const QUrl
> &historyUrl)
> 
> Example:
> 
> setHtml("<html><body><img src="kenneth/image.png"></body></html>",
> QUrl("file:///home"));
> 
> This will show a page with an image /home/kenneth/image.png and it will add
> file:///home to the history
> 
> That seems like unintended behaviour
> 
> 
> On the other hand I might want the ability to do setHtml("<html><body><img
> src="kenneth/image.png"></body></html>", QUrl("file:///home"),
> QUrl("about:arora"));
> 
> So it will add about:arora to the history and forward/back history

[Kenneth Rohde Christiansen]

After talking to Simon, we agree that it is questionable whether the setHtml()
should add the base url to the history or not, but it is another discussion
that can take place on another bug report.

[Simon Hausmann]

Discussing this with Kent just gave me an alternate idea to fix the API:

setHtml("<html>...", QUrl("file://..."), QUrl("...");

is not very readable. I don't know what the difference between the second and third argument is. I would prefer something slightly more readable:

QWebLoadRequest request;
request.baseUrl = "file://....";
request.mimeType = "text/html";
request.contentData = "<html>..";
request.historyURl = "about:foo";
webFrame->load(request);

[Kenneth Rohde Christiansen]

(In reply to comment #30)
> Discussing this with Kent just gave me an alternate idea to fix the API:

Kenneth you mean ;-)

> setHtml("<html>...", QUrl("file://..."), QUrl("...");
> 
> is not very readable. I don't know what the difference between the second and
> third argument is. I would prefer something slightly more readable:
> 
> QWebLoadRequest request;
> request.baseUrl = "file://....";
> request.mimeType = "text/html";
> request.contentData = "<html>..";
> request.historyURl = "about:foo";
> webFrame->load(request);

We could even make QWebLoadRequest inherit from QNetworkRequest!

It is a good idea and it is quite extensible.

[Antonio Gomes]

(In reply to comment #31)
> (In reply to comment #30)
> > Discussing this with Kent just gave me an alternate idea to fix the API:
> 
> Kenneth you mean ;-)
> 
> > setHtml("<html>...", QUrl("file://..."), QUrl("...");
> > 
> > is not very readable. I don't know what the difference between the second and
> > third argument is. I would prefer something slightly more readable:
> > 
> > QWebLoadRequest request;
> > request.baseUrl = "file://....";
> > request.mimeType = "text/html";
> > request.contentData = "<html>..";
> > request.historyURl = "about:foo";
> > webFrame->load(request);
> 
> We could even make QWebLoadRequest inherit from QNetworkRequest!
> 
> It is a good idea and it is quite extensible.

seconded. i will try out something in that sense.

[Antonio Gomes]

Created attachment 39950 [details]
patch 0.3 - make setHtml and setContent to not change history

as per API review on IRC, it was decided to make setHtml and setContent not to change session and global history at all.

patch 0.3 implements so and adds an autotest that it by calling qwebframe::setHtml in which an image is loaded (baseUrl used) and checks if history has changed (page()->history()->count == 0).

patches for other features/improvements in followup bugs.

[Antonio Gomes]

Created attachment 40045 [details]
patch 0.3.1 - make setHtml and setContent to not change history 

* updated to tot.
* fixed typo in a method header block.

[Antonio Gomes]

Created attachment 40063 [details]
patch 0.3.2 - make setHtml and setContent to not change history   (2

same as previous patch, but fixed another typo.

[Simon Hausmann]

(In reply to comment #35)
> Created an attachment (id=40063) [details]
> patch 0.3.2 - make setHtml and setContent to not change history   (2
> 
> same as previous patch, but fixed another typo.

I think this patch looks good, but I heard concerns that this change might break Arora. Kenneth?

[Antonio Gomes]

(In reply to comment #36)
> (In reply to comment #35)
> > Created an attachment (id=40063) [details] [details]
> > patch 0.3.2 - make setHtml and setContent to not change history   (2
> > 
> > same as previous patch, but fixed another typo.
> 
> I think this patch looks good, but I heard concerns that this change might
> break Arora. Kenneth?

simon, this change alone will break Arora, yeah. The "fix" for Arora would be make use of stuff in bug 29731 , which this one is a blocker of.

[Tor Arne Vestbø]

Comment on attachment 40063 [details]
patch 0.3.2 - make setHtml and setContent to not change history   (2

> +  \note Any call to setHtml will not change neither session nor global history for this frame.
> +

Move this below the next block

>    When using this method WebKit assumes that external resources such as JavaScript programs or style
>    sheets are encoded in UTF-8 unless otherwise specified. For example, the encoding of an external
>    script can be specified through the charset attribute of the HTML script tag. It is also possible
>    for the encoding to be specified by web server.

<Here> :) And word it something like:

\note This method will not affect session or global history for the frame.

>  
> -  \sa toHtml()
> +  \sa toHtml(), setContent()
>  */
>  void QWebFrame::setHtml(const QString &html, const QUrl &baseUrl)
>  {
> @@ -718,7 +720,7 @@ void QWebFrame::setHtml(const QString &html, const QUrl &baseUrl)
>      WebCore::ResourceRequest request(kurl);
>      const QByteArray utf8 = html.toUtf8();
>      WTF::RefPtr<WebCore::SharedBuffer> data = WebCore::SharedBuffer::create(utf8.constData(), utf8.length());
> -    WebCore::SubstituteData substituteData(data, WebCore::String("text/html"), WebCore::String("utf-8"), kurl);
> +    WebCore::SubstituteData substituteData(data, WebCore::String("text/html"), WebCore::String("utf-8"), KURL());
>      d->frame->loader()->load(request, substituteData, false);
>  }
>  
> @@ -731,7 +733,9 @@ void QWebFrame::setHtml(const QString &html, const QUrl &baseUrl)
>  
>    The \a data is loaded immediately; external objects are loaded asynchronously.
>  
> -  \sa toHtml()
> +  \note Any call to setContent will not change neither session nor global history for this frame.
> +
> +  \sa toHtml(), setHtml()

same

> +DEFINES += RESOURCES_PATH=$$PWD/resources

Is this really needed?

[Antonio Gomes]

Created attachment 40115 [details]
committed r48972: patch 0.3.3 - make setHtml and setContent to not change history

comments addressed

> > +DEFINES += RESOURCES_PATH=$$PWD/resources
> 
> Is this really needed?

as you suggested, i changed this to be as qtextbrowser in qt.

[Kenneth Rohde Christiansen]

I have a new idea for an API that would solve our problems.

On IRC we had a discussion today, following my making some tests. 

The conclusion was that we should use the infrastructure of WebCore.

Adding a QWebHistory::addUrl(...) would *not* solve our problems as

- the setHtml would send en emit urlChanged(QUrl("about:blank")
- the constructed history item would need to set valid "lastVisitedDate", etc as well as the title which is supplied with the HTML in the forms of the <title> tag.
- it might break with WebCore changes
- all other ports use the fallbackUrl supplied by SubstituteData.

The Qt developers have expressed the dislike of adding a historyUrl or fallbackUrl to the setHtml as two followed QUrl arguments make it hard to read code using it.

Instead I suggest the following API:


/*!
 Loads a new page with the specified \a html. 

 This method is meant for dealing with failed loads and similar situations,
 where alternative \a html is shown, but when revisited or reloaded the
 \a fallbackUrl should be loaded instead.

 External objects such as stylesheets or images referenced in the HTML document
 are located relative to baseUrl which is optional.

 The \a html is loaded immediately; external objects are loaded asynchronously.

 When using this method, WebKit assumes that external resources such as
 JavaScript programs or style sheets are encoded in UTF-8 unless otherwise
 specified. For example, the encoding of an external script can be specified
 through the charset attribute of the HTML script tag. Alternatively, the
 encoding can also be specified by the web server.

 \sa load(), setContent(), setHtml(), QWebFrame::toHtml()
*/

loadAlternativeHtml(const QUrl& fallbackUrl, const QString& html, const QUrl& baseUrl = QUrl())
{
   ...
}


Comments?

[Antonio Gomes]

Created attachment 40139 [details]
implemented loadAlternativeHtml + autotest

patch on top of patch 0.3.3

[Kenneth Rohde Christiansen]

(In reply to comment #41)
> Created an attachment (id=40139) [details]
> implemented loadAlternativeHtml + autotest

About the method name: 

I chose using "load" to indicate that it creates a new page in history etc, as all other load methods, and in contract to setHtml that just "sets" (substitutes) the DOM with the new HTML.

As it is kind of specialized it was only added to QWebFrame, The "alternative" makes it clear to the developer that this function is meant for special cases. The name is also very similar to what Apple uses in their API.

[Antonio Gomes]

it might also worth to mention the fact that MAC port also adopts similar  naming for similar methods:

loadData
loadHTMLString
loadAlternateHTMLString   <---------

[Kenneth Rohde Christiansen]

(In reply to comment #39)
> Created an attachment (id=40115) [details]
> patch 0.3.3 - make setHtml and setContent to not change history
> 
> comments addressed
> 
> > > +DEFINES += RESOURCES_PATH=$$PWD/resources
> > 
> > Is this really needed?
> 
> as you suggested, i changed this to be as qtextbrowser in qt.

As all comments have been fixed and since we have agreed that this is the right change (see below) I believe this should be simple to r+ by any reviewer.

We found out earlier that the Arora browser was depending on the broken behaviour, but a recent commit by me, has made it possible to obtain the same behaviour using an error page extension to QWebPage.  

> On Friday 18 September 2009 Hausmann Simon (Nokia-D-Qt/Oslo), wrote:
>>     * https://bugs.webkit.org/show_bug.cgi?id=29248 (setHtml behaviour,
>> invisible loads)
>
> We determined that the current behaviour of QWebFrame::setHtml() is broken in
> the sense that _if_ a base url is specified then an entry is added to the
> back/forward history. If no base url is specified then the history remains
> unchanged.
>
> So we decided to change setHtml() to _never_ change the history and clarify
> this in the docs.
>
> Antonio is looking into making auto tests for the various issues he found,
> including the current setHtml() problem. Autotests will make it easier to
> understand and distinguish the issues :)

[Kenneth Rohde Christiansen]

(In reply to comment #43)
> it might also worth to mention the fact that MAC port also adopts similar 
> naming for similar methods:
> 
> loadData
> loadHTMLString
> loadAlternateHTMLString   <---------

We have decided against adding this method.

[Kenneth Rohde Christiansen]

Comment on attachment 40139 [details]
implemented loadAlternativeHtml + autotest

This has been implemented in another way, using a QWebPage ErrorPage extension.

[Antonio Gomes]

fixed. r48972

[Antonio Gomes]

Comment on attachment 40115 [details]
committed r48972: patch 0.3.3 - make setHtml and setContent to not change history

clearing r+ flag since patch has landed.

[Antonio Gomes]

and missing resource late committed r50217
	A	WebKit/qt/tests/qwebframe/resources/image2.png
	M	WebKit/qt/ChangeLog

this should make autotests to pass again