Bug 79495 – [GTK] FrameLoader signals: gtk-doc fixes

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Philippe Normand]

+++ This bug was initially created as a clone of Bug #49543 +++

See https://bugs.webkit.org/show_bug.cgi?id=49543#c36

[Carlos Garcia Campos]

I've just added this to http://trac.webkit.org/wiki/WebKitGTK/1.8.x 

I think someone should fix this before 1.8, or roll the patch out. Note that the bug is not just about gtk-doc syntax errors.

[Martin Robinson]

(In reply to comment #1)
> I've just added this to http://trac.webkit.org/wiki/WebKitGTK/1.8.x 
> 
> I think someone should fix this before 1.8, or roll the patch out. Note that the bug is not just about gtk-doc syntax errors.

From your comment, you found both gtkdoc syntax errors and places where the documentation could be improved. Were there other problems as well?

[Carlos Garcia Campos]

The main problem for me with the patch is the lack of unit tests, but I would be happy just if the documentation is fixed.

[Philippe Normand]

(In reply to comment #3)
> The main problem for me with the patch is the lack of unit tests, but I would be happy just if the documentation is fixed.

That patch unskipped a good number of layout tests though and that new API is used in DRT. I don't see the point of adding unit tests in that case.

[Philippe Normand]

Ok let's do this..

[Carlos Garcia Campos]

(In reply to comment #4)
> (In reply to comment #3)
> > The main problem for me with the patch is the lack of unit tests, but I would be happy just if the documentation is fixed.
> 
> That patch unskipped a good number of layout tests though and that new API is used in DRT. I don't see the point of adding unit tests in that case.

If the patch had included unit tests, you would have noticed that length-received is not the length of the resource, but the length of the data chunk received and that it can be emitted multiple times, for example. Unit test are useful to make sure that API works for multiple uses, I haven't looked at how DRT use the API, but I guess it doesn't use all the API (signals and methods) exposed.

[Philippe Normand]

Created attachment 130319 [details]
Patch

[Carlos Garcia Campos]

Comment on attachment 130319 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=130319&action=review

Thanks for fixing this.

> Source/WebKit/gtk/webkit/webkitwebframe.cpp:400
> +     * Emitted when new resource data has been received. The
> +     * @length_received variable stores the amount of bytes received so
> +     * far. This is useful to provide progress information about the

No, the value is not accumulated, it's the length of the chunk received, no the total amount of data received so far.

[Martin Robinson]

(In reply to comment #6)
 
> If the patch had included unit tests, you would have noticed that length-received is not the length of the resource, but the length of the data chunk received and that it can be emitted multiple times, for example. Unit test are useful to make sure that API works for multiple uses, I haven't looked at how DRT use the API, but I guess it doesn't use all the API (signals and methods) exposed.

If there is no layout test covering this, then it makes sense to add one.

[Carlos Garcia Campos]

(In reply to comment #9)
> (In reply to comment #6)
> 
> > If the patch had included unit tests, you would have noticed that length-received is not the length of the resource, but the length of the data chunk received and that it can be emitted multiple times, for example. Unit test are useful to make sure that API works for multiple uses, I haven't looked at how DRT use the API, but I guess it doesn't use all the API (signals and methods) exposed.
> 
> If there is no layout test covering this, then it makes sense to add one.

It's my opinion, I'm not saying phil did wrong not including unit tests in the patch, I just think that all patches adding new API should include unit tests even if the API is used by other tests. Typically the only tests that check every method, signal, etc. exposed by the API are unit tests. I think layout tests are complementary to unit tests. I can be wrong of course.

[Martin Robinson]

Comment on attachment 130319 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=130319&action=review

>> Source/WebKit/gtk/webkit/webkitwebframe.cpp:400
>> +     * far. This is useful to provide progress information about the
> 
> No, the value is not accumulated, it's the length of the chunk received, no the total amount of data received so far.

I'll fix this and land the patch.

[Martin Robinson]

(In reply to comment #11)
> (From update of attachment 130319 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=130319&action=review
> 
> >> Source/WebKit/gtk/webkit/webkitwebframe.cpp:400
> >> +     * far. This is useful to provide progress information about the
> > 
> > No, the value is not accumulated, it's the length of the chunk received, no the total amount of data received so far.
> 
> I'll fix this and land the patch.

I think it makes sense to rename this signal. It seems that the name resource-content-length-received is a weird side effect of the Objective-C naming scheme. It's very odd! I think a better name would be resource-content-received. The name resource-content-length-received suggests that the Content-Length header of the HTTP header has been parsed, so I think it could be quite confusing.

[Martin Robinson]

(In reply to comment #12)

> I think it makes sense to rename this signal. It seems that the name resource-content-length-received is a weird side effect of the Objective-C naming scheme. It's very odd! I think a better name would be resource-content-received. The name resource-content-length-received suggests that the Content-Length header of the HTTP header has been parsed, so I think it could be quite confusing.

Well, it looks like we've already pushed this in 1.7.5, so we cannot change the signal name now. We should just verify that we fix the issue in WebKit2, I guess.

[Martin Robinson]

Committed r109997: <http://trac.webkit.org/changeset/109997>

[Carlos Garcia Campos]

(In reply to comment #12)
> (In reply to comment #11)
> > (From update of attachment 130319 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=130319&action=review
> > 
> > >> Source/WebKit/gtk/webkit/webkitwebframe.cpp:400
> > >> +     * far. This is useful to provide progress information about the
> > > 
> > > No, the value is not accumulated, it's the length of the chunk received, no the total amount of data received so far.
> > 
> > I'll fix this and land the patch.
> 
> I think it makes sense to rename this signal. It seems that the name resource-content-length-received is a weird side effect of the Objective-C naming scheme. It's very odd! I think a better name would be resource-content-received. The name resource-content-length-received suggests that the Content-Length header of the HTTP header has been parsed, so I think it could be quite confusing.

Yes, that's exactly why used a different name in wk2. Both Downloads API and Resources patch use received-data

[Carlos Garcia Campos]

(In reply to comment #13)
> (In reply to comment #12)
> 
> > I think it makes sense to rename this signal. It seems that the name resource-content-length-received is a weird side effect of the Objective-C naming scheme. It's very odd! I think a better name would be resource-content-received. The name resource-content-length-received suggests that the Content-Length header of the HTTP header has been parsed, so I think it could be quite confusing.
> 
> Well, it looks like we've already pushed this in 1.7.5, so we cannot change the signal name now. We should just verify that we fix the issue in WebKit2, I guess.

In wk2 it's called received-data for consistency with the downloads API.