| Summary: | [GTK] FrameLoader signals: gtk-doc fixes | ||||||
|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Philippe Normand <pnormand> | ||||
| Component: | WebKitGTK | Assignee: | Philippe Normand <pnormand> | ||||
| Status: | RESOLVED FIXED | ||||||
| Severity: | Normal | CC: | cgarcia, mrobinson | ||||
| Priority: | P2 | ||||||
| Version: | 528+ (Nightly build) | ||||||
| Hardware: | PC | ||||||
| OS: | Linux | ||||||
| Attachments: |
|
||||||
|
Description
Philippe Normand
2012-02-24 08:59:56 PST
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. (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? 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. (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. Ok let's do this.. (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. Created attachment 130319 [details]
Patch
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. (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. (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. 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. (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. (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. Committed r109997: <http://trac.webkit.org/changeset/109997> (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 (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. |