Bug 14815 – [gtk] API implementation: reload

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

[Xan Lopez]

Add the webkit_gtk_page_reload method.

[Xan Lopez]

Created attachment 15747 [details]
Implement reload

[Holger Freyther]

Looks good two minor things:
   -As of yesterday we have decided to use the WebKit CodingStyle in the WebKit/Gtk API implementation. E.g use camelCase for variable names. This makes private_data -> privateData and frame_data -> frameData and we need to do that for the rest of the implementation as well (but in another patch)

   -Did you use WebKitTools/Scripts/prepare-ChangeLog to create the ChangeLog entry?

[Holger Freyther]

Oh one more CodingStyle issue (sorry):
+webkit_gtk_page_reload (WebKitGtkPage *page); it should be WebKitGtkPage* page in both the header and the implementation. In theory the header could be different but currently all other methods use WebKitGtkPage*, we can decide to change that for the header but this would need to be handled in a different bug.

[Xan Lopez]

(In reply to comment #2)
> Looks good two minor things:
>    -As of yesterday we have decided to use the WebKit CodingStyle in the
> WebKit/Gtk API implementation. E.g use camelCase for variable names. This makes
> private_data -> privateData and frame_data -> frameData and we need to do that
> for the rest of the implementation as well (but in another patch)

ACK.

> 
>    -Did you use WebKitTools/Scripts/prepare-ChangeLog to create the ChangeLog
> entry?

Nope, is it mandatory as long as the output is OK?

> +webkit_gtk_page_reload (WebKitGtkPage *page); it should be WebKitGtkPage* page
> in both the header and the implementation. In theory the header could be
> different but currently all other methods use WebKitGtkPage*, we can decide to
> change that for the header but this would need to be handled in a different
> bug.

Ok, GTK+ uses the type *foo style, but I have no strong feelings about it. The only real benefit I can see is more clarity when declaring more than one variable in one line.

[Holger Freyther]

(In reply to comment #4)
> (In reply to comment #2)
> Nope, is it mandatory as long as the output is OK?
> 

Well, the script creates "Reviewed by NOBODY (OOPS)" and webkit.org has a pre-commit hook to check ChangeLogs for OOPS. So If I forget to add my name to the line I'm going to commit a 'bad' changelog entry.
Try the script, if you don't like it we can live with that.

[Xan Lopez]

(In reply to comment #5)
> Well, the script creates "Reviewed by NOBODY (OOPS)" and webkit.org has a
> pre-commit hook to check ChangeLogs for OOPS. So If I forget to add my name to
> the line I'm going to commit a 'bad' changelog entry.
> Try the script, if you don't like it we can live with that.

I know the script, I used to use it before I switched to git/git-svn for most things. These days I use moap changelog prepare or simply C-x 4 a in emacs, but anyway I don't if the only matter is the OOPS stuff that has an easy solution. I think the output from moap + the OOPS for unreviewed is identical to the stuff produced by prepare-Changelog.

[Mark Rowe (bdash)]

Comment on attachment 15747 [details]
Implement reload

+void webkit_gtk_page_reload (WebKitGtkPage *page)

The whitespace in this line is inconsistent with other methods in the implementation file, and should be fixed by whoever lands this.

It would also be preferable if you could leave the "Reviewed by NOBODY (OOPS!)" line intact in the ChangeLog.  We have post-commit hooks that key off this to ensure that a reviewer is added.

[Xan Lopez]

(In reply to comment #7)
> (From update of attachment 15747 [details] [edit])
> +void webkit_gtk_page_reload (WebKitGtkPage *page)
> 
> The whitespace in this line is inconsistent with other methods in the
> implementation file, and should be fixed by whoever lands this.
> 
> It would also be preferable if you could leave the "Reviewed by NOBODY (OOPS!)"
> line intact in the ChangeLog.  We have post-commit hooks that key off this to
> ensure that a reviewer is added.
> 


Noted, thanks for the review. Can we get this in?

[Alp Toker]

Landed in r24911