293670 - pages rendered with gtkmozembed stream API don't fire onload

Status: RESOLVED FIXED

(Core Graveyard :: Embedding: GTK Widget, defect)

Description

[Christian Persch (GNOME) (away; not receiving bug mail)]

Steps to reproduce:
0) Apply the attached patch to TestGtkEmbed, compile & run
1) Chose "Test stream" from File menu

Expected results:
Alert box.

Actual results:
No alert.

Comment 1

[Christian Persch (GNOME) (away; not receiving bug mail)]

Attachment: testcase as patch against TestGtkEmbed

Comment 2

[Don Scorgie]

Hi,

This appears to be causing a11y problems for programs using the stream api.

See:
http://bugzilla.gnome.org/show_bug.cgi?id=157941

Comment 3

[Ginn Chen]

The difference between "gtk_moz_embed_load_url" method and "gtk_moz_embed_open_stream" is "gtk_moz_embed_open_stream" creates new load group without observer.
See nsEmbedStream::OpenStream(nsIURI *aBaseURI, const nsACString& aContentType)
116   // create a new load group
117   rv = NS_NewLoadGroup(getter_AddRefs(mLoadGroup), nsnull);
118   if (NS_FAILED(rv))
119     return rv;

Instead, "gtk_moz_embed_load_url" uses nsDocShell.

This causes the yelp a11y bug (http://bugzilla.gnome.org/show_bug.cgi?id=157941).

I'm wondering if it is the same cause of not firing onload.
If so, how can we fix it?

Comment 4

[Boris Zbarsky [:bzbarsky]]

> I'm wondering if it is the same cause of not firing onload.

Yes.  Onload is fired by the load group.

> If so, how can we fix it?

Perhaps EmbedStream should stop doing what it's doing and switch to nsIDocShell.loadStream?  I really don't like having these content-viewer-embedding stuff in EmbedStream.  Then again, this may not work well with AppendToStream, etc.

It could also do something akin to what nsDocShell::LoadStream does, but with its own channel.

Failing tall hat, it can get the right loadgroup off the docshell.

All this depends on what the function is _trying_ to do, of course.

Comment 5

[Hector Chu]

Attachment: Patch

This patch fixes the following:
1. Load events now fire.
2. An unlimited amount of data may be appended without blocking.
3. If openStream fails it can always be tried again.
4. The streamed document becomes part of the history.
I have also verified that if the content type includes a charset it will work as expected.

Comment 6

[Hector Chu]

Bug 495055 will also be fixed by this patch.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 386000 [details] [diff] [review]
Patch

Looks fine, assuming the file is supposed to be using tabs for indent.  If not, fix that, please.

Comment 8

[Hector Chu]

Boris, just to be sure, do you want me to convert the tabs to spaces?

Comment 9

[Boris Zbarsky [:bzbarsky]]

I want you to be consistent with the existing indentation in the file.  If you're removing all existing longer-than-2 indentation, please do convert to spaces and add a corresponding modeline to the file.

Comment 10

[Hector Chu]

Attachment: Patch (same but with spaces instead of tabs and modeline)

Comment 11

[Hector Chu]

Boris, assuming the revised patch is OK can you push this for me please?

Comment 12

[Boris Zbarsky [:bzbarsky]]

Pushed http://hg.mozilla.org/mozilla-central/rev/05fba5eb6b42

Comment 13

[Ginn Chen]

This caused a huge regression of yelp.
See https://bugzilla.gnome.org/show_bug.cgi?id=603561

I'm not sure what is the root cause, since TestGtkEmbed is still working in mozilla-1.9.2.

Comment 14

[Ginn Chen]

just noticed, it was already fixed by
http://git.gnome.org./browse/yelp/commit/?id=a5588114ed94d00ca64913aa5b248e09a5e13edc