Bug 62533 - [EFL] Fix and update doxygen documentation for ewk_frame
Summary: [EFL] Fix and update doxygen documentation for ewk_frame
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit EFL (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Minor
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-06-13 00:07 PDT by Grzegorz Czajkowski
Modified: 2011-06-15 09:45 PDT (History)
7 users (show)

See Also:


Attachments
proposed patch (46.56 KB, patch)
2011-06-13 00:12 PDT, Grzegorz Czajkowski
eric: review-
Details | Formatted Diff | Diff
updated patch according to Raphael's suggestions (44.25 KB, patch)
2011-06-14 00:18 PDT, Grzegorz Czajkowski
no flags Details | Formatted Diff | Diff
updated patch (43.93 KB, patch)
2011-06-14 01:02 PDT, Grzegorz Czajkowski
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Grzegorz Czajkowski 2011-06-13 00:07:53 PDT
This patch:
 - fixes method descriptions,
 - adds briefs for structure and typdef,
 - replaces NULL to 0 in doxygen documentation (webkit's style required it),
 - removes dotes from the end of params and return description (see EFL's documentation),
 - moves includes and defines below brief of ewk_frame.h
 - moves internal methods at the end of ewk_frame.cpp

It will be nice to replace a brief of structure and typedef from
/// Represents actions of touch events (C++ style),
to
//! Represents actions of touch events (C style).
webkit's style treats this kind of comment as a generall comment and it causes problems.

I would like to know your opinion about it.
Comment 1 Grzegorz Czajkowski 2011-06-13 00:12:25 PDT
Created attachment 96930 [details]
proposed patch
Comment 2 Raphael Kubo da Costa (:rakuco) 2011-06-13 06:36:41 PDT
> Source/WebKit/efl/ChangeLog:12
> +         - removes dotes from the end of params and return description (see EFL's documentation),

dotes -> dots (or full stops)

> Source/WebKit/efl/ChangeLog:14
> +         - moves internal methods at the end of ewk_frame.cpp.

at the end -> to the end

> Source/WebKit/efl/ewk/ewk_frame.cpp:377
> + * Finds a child frame by it's name, recursively.

Wrong change :)

> Source/WebKit/efl/ewk/ewk_frame.cpp:425
> + * It returns a internal string and should not

a internal -> an internal

> Source/WebKit/efl/ewk/ewk_frame.cpp:441
> + * It returns a internal string and should not

ditto

> Source/WebKit/efl/ewk/ewk_frame.cpp:457
> + * It returns a internal string and should not

ditto

> Source/WebKit/efl/ewk/ewk_frame.cpp:541
> + * @param contents what to load into frame, must @b not be @c 0

It can be 0, the function will just return EINA_FALSE if so.

> Source/WebKit/efl/ewk/ewk_frame.cpp:681
>   * Selects everything.

Even though you have not written this line yourself, I'd rather make it more specific than "everything".

> Source/WebKit/efl/ewk/ewk_frame.cpp:809
> + * Sets if should highlight matches marked with ewk_frame_text_matches_mark().

I'd change it to "Sets whether matches marked with ewk_frame_text_matches_mark() should be highlighted".

> Source/WebKit/efl/ewk/ewk_frame.cpp:825
> + * Gets if should highlight matches marked with ewk_frame_text_matches_mark().

I'd change it to "Returns whether matches marked with ewk_frame_text_matches_mark() should be highlighted".

> Source/WebKit/efl/ewk/ewk_frame.cpp:829
> + * @return @c EINA_TRUE if matches are highlighted, @c EINA_FALSE otherwise

Either change "are" to to "should" here, or do the opposite above.

> Source/WebKit/efl/ewk/ewk_frame.cpp:855
> + * Gets position of n-th matched text in the frame.

I'd change it to "Returns the position of the n-th matched text in the frame".

> Source/WebKit/efl/ewk/ewk_frame.cpp:983
> + * Queries if is possible to navigate backwards one item in the history.

if is -> if it is

> Source/WebKit/efl/ewk/ewk_frame.cpp:987
> + * @return @c EINA_TRUE if is possible to navigate backward one item in the history, @c EINA_FALSE otherwise

if is -> if it is
backward -> backwards

> Source/WebKit/efl/ewk/ewk_frame.cpp:997
> + * Queries if is possible to navigate forwards one item in the history.

ditto

> Source/WebKit/efl/ewk/ewk_frame.cpp:1001
> + * @return @c EINA_TRUE if is possible to navigate forwards in the history, @c EINA_FALSE otherwise

ditto

> Source/WebKit/efl/ewk/ewk_frame.cpp:1011
> + * Queries if is possible to navigate given @a steps in the history.

iddot

> Source/WebKit/efl/ewk/ewk_frame.cpp:1017
> + * @return @c EINA_TRUE if is possible to navigate @a steps in the history, @c EINA_FALSE otherwise

ditto

> Source/WebKit/efl/ewk/ewk_frame.cpp:1028
> + * Gets current zoom level used by this frame.

gets the

> Source/WebKit/efl/ewk/ewk_frame.cpp:1045
> + * Sets current zoom level used by this frame.

sets the

> Source/WebKit/efl/ewk/ewk_frame.cpp:1105
> + * @param hit_test instance, must @b not be @c 0

It can actually be 0, the function will just return in this case.

> Source/WebKit/efl/ewk/ewk_frame.cpp:1187
> + * Sets a relative scroll of the given frame.

There could be a more detailed description here, stating that this function will scroll  N pixels from the current location.

> Source/WebKit/efl/ewk/ewk_frame.cpp:1206
> + * Sets an absolute scroll of the given frame.

Ditto.

> Source/WebKit/efl/ewk/ewk_frame.cpp:1329
> + * It gets paintsEntireContents flag. This flag tells if dirty areas 

IMO, paintsEntireContents should not be mentioned at all, as it is an implementation detail. Just explaining what the returned value means should be enough.

> Source/WebKit/efl/ewk/ewk_frame.cpp:1348
> + * It sets paintsEntireContents flag. This flag tells if dirty areas 

Ditto.

> Source/WebKit/efl/ewk/ewk_frame.cpp:1382
> + * Currently feature <b>is not implemented</b>. What should be done on focus out?

I don't think this line should be added.

> Source/WebKit/efl/ewk/ewk_frame.h:75
> + * \brief   Structure used to report load errors.

@brief for consistency with the rest?

> Source/WebKit/efl/ewk/ewk_frame.h:96
> + * \brief   Structure used to report resource request.

ditto
Comment 3 Eric Seidel (no email) 2011-06-13 14:41:20 PDT
Comment on attachment 96930 [details]
proposed patch

rs=me.
Comment 4 Eric Seidel (no email) 2011-06-13 14:41:49 PDT
Comment on attachment 96930 [details]
proposed patch

Looks like you had a real review, they just didn't set the r-.
Comment 5 Grzegorz Czajkowski 2011-06-14 00:18:43 PDT
Created attachment 97078 [details]
updated patch according to Raphael's suggestions
Comment 6 Grzegorz Czajkowski 2011-06-14 00:29:17 PDT
(In reply to comment #2)
> > Source/WebKit/efl/ChangeLog:12
> > +         - removes dotes from the end of params and return description (see EFL's documentation),

Thanks for your review.

> 
> dotes -> dots (or full stops)

Fixed.

> 
> > Source/WebKit/efl/ChangeLog:14
> > +         - moves internal methods at the end of ewk_frame.cpp.
> 
> at the end -> to the end

Fixed.

> 
> > Source/WebKit/efl/ewk/ewk_frame.cpp:377
> > + * Finds a child frame by it's name, recursively.
> 
> Wrong change :)

Finds a child frame by name 
or
Finds a child frame by it name 

I used second one. Do you agree? :)

> 
> > Source/WebKit/efl/ewk/ewk_frame.cpp:425
> > + * It returns a internal string and should not
> 
> a internal -> an internal

Fixed in all cases,

> 
> > Source/WebKit/efl/ewk/ewk_frame.cpp:441
> > + * It returns a internal string and should not
> 
> ditto
> 
> > Source/WebKit/efl/ewk/ewk_frame.cpp:457
> > + * It returns a internal string and should not
> 
> ditto
> 
> > Source/WebKit/efl/ewk/ewk_frame.cpp:541
> > + * @param contents what to load into frame, must @b not be @c 0
> 
> It can be 0, the function will just return EINA_FALSE if so.

Removed ", must @b not be @c 0"

> 
> > Source/WebKit/efl/ewk/ewk_frame.cpp:681
> >   * Selects everything.
> 
> Even though you have not written this line yourself, I'd rather make it more specific than "everything".
> 
> > Source/WebKit/efl/ewk/ewk_frame.cpp:809
> > + * Sets if should highlight matches marked with ewk_frame_text_matches_mark().
> 
> I'd change it to "Sets whether matches marked with ewk_frame_text_matches_mark() should be highlighted".

Changed

> 
> > Source/WebKit/efl/ewk/ewk_frame.cpp:825
> > + * Gets if should highlight matches marked with ewk_frame_text_matches_mark().
> 
> I'd change it to "Returns whether matches marked with ewk_frame_text_matches_mark() should be highlighted".

Changed to: Returns whether matches marked with ewk_frame_text_matches_mark() are highlighted.

> > Source/WebKit/efl/ewk/ewk_frame.cpp:829
> > + * @return @c EINA_TRUE if matches are highlighted, @c EINA_FALSE otherwise
> 
> Either change "are" to to "should" here, or do the opposite above.

Changed "are" above.

> 
> > Source/WebKit/efl/ewk/ewk_frame.cpp:855
> > + * Gets position of n-th matched text in the frame.
> 
> I'd change it to "Returns the position of the n-th matched text in the frame".

Changed

> > Source/WebKit/efl/ewk/ewk_frame.cpp:983
> > + * Queries if is possible to navigate backwards one item in the history.
> 
> if is -> if it is

Replaces "if is" to "if it's" in all history methods.

> 
> > Source/WebKit/efl/ewk/ewk_frame.cpp:987
> > + * @return @c EINA_TRUE if is possible to navigate backward one item in the history, @c EINA_FALSE otherwise
> 
> if is -> if it is
> backward -> backwards

Fixed

> 
> > Source/WebKit/efl/ewk/ewk_frame.cpp:997
> > + * Queries if is possible to navigate forwards one item in the history.
> 
> ditto
> 
> > Source/WebKit/efl/ewk/ewk_frame.cpp:1001
> > + * @return @c EINA_TRUE if is possible to navigate forwards in the history, @c EINA_FALSE otherwise
> 
> ditto
> 
> > Source/WebKit/efl/ewk/ewk_frame.cpp:1011
> > + * Queries if is possible to navigate given @a steps in the history.
> 
> iddot
> 
> > Source/WebKit/efl/ewk/ewk_frame.cpp:1017
> > + * @return @c EINA_TRUE if is possible to navigate @a steps in the history, @c EINA_FALSE otherwise
> 
> ditto
> 
> > Source/WebKit/efl/ewk/ewk_frame.cpp:1028
> > + * Gets current zoom level used by this frame.
> 
> gets the
> 
> > Source/WebKit/efl/ewk/ewk_frame.cpp:1045
> > + * Sets current zoom level used by this frame.
> 
> sets the
> 
> > Source/WebKit/efl/ewk/ewk_frame.cpp:1105
> > + * @param hit_test instance, must @b not be @c 0
> 
> It can actually be 0, the function will just return in this case.

Removed ", must @b not be @c 0"
 
> > Source/WebKit/efl/ewk/ewk_frame.cpp:1187
> > + * Sets a relative scroll of the given frame.
> 
> There could be a more detailed description here, stating that this function will scroll  N pixels from the current location.

Added an additional comment:
This function does scroll @a dx and @a dy pixels from the current position of scroll.

> 
> > Source/WebKit/efl/ewk/ewk_frame.cpp:1206
> > + * Sets an absolute scroll of the given frame.
> 
> Ditto.
> 
> > Source/WebKit/efl/ewk/ewk_frame.cpp:1329
> > + * It gets paintsEntireContents flag. This flag tells if dirty areas 
> 
> IMO, paintsEntireContents should not be mentioned at all, as it is an implementation detail. Just explaining what the returned value means should be enough.

Ok.

> 
> > Source/WebKit/efl/ewk/ewk_frame.cpp:1348
> > + * It sets paintsEntireContents flag. This flag tells if dirty areas 
> 
> Ditto.
> 
> > Source/WebKit/efl/ewk/ewk_frame.cpp:1382
> > + * Currently feature <b>is not implemented</b>. What should be done on focus out?
> 
> I don't think this line should be added.

Removed

> 
> > Source/WebKit/efl/ewk/ewk_frame.h:75
> > + * \brief   Structure used to report load errors.
> 
> @brief for consistency with the rest?
> 
> > Source/WebKit/efl/ewk/ewk_frame.h:96
> > + * \brief   Structure used to report resource request.
> 
> ditto


Replaced to @brief
Comment 7 Grzegorz Czajkowski 2011-06-14 01:02:44 PDT
Created attachment 97080 [details]
updated patch

Restores origin ewk_frame_child_find description.
Updated patch to https://bugs.webkit.org/show_bug.cgi?id=62365
Comment 8 Raphael Kubo da Costa (:rakuco) 2011-06-14 05:44:40 PDT
Informal r+ from my side.
Comment 9 Antonio Gomes 2011-06-15 08:11:24 PDT
Comment on attachment 97080 [details]
updated patch

CQ+ it when it gets API reviewed, please.
Comment 10 Raphael Kubo da Costa (:rakuco) 2011-06-15 09:04:32 PDT
I've reviewed it a few days ago, but can't cq+ it.
Comment 11 Lucas De Marchi 2011-06-15 09:06:09 PDT
(In reply to comment #10)
> I've reviewed it a few days ago, but can't cq+ it.

I think it will conflict with recent changes to that file, but let's try.
Comment 12 WebKit Review Bot 2011-06-15 09:45:23 PDT
Comment on attachment 97080 [details]
updated patch

Clearing flags on attachment: 97080

Committed r88947: <http://trac.webkit.org/changeset/88947>
Comment 13 WebKit Review Bot 2011-06-15 09:45:29 PDT
All reviewed patches have been landed.  Closing bug.