Bug 62533 – [EFL] Fix and update doxygen documentation for ewk_frame

• Component: WebKit EFL
• Status: RESOLVED FIXED

[Grzegorz Czajkowski]

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.

[Grzegorz Czajkowski]

Created attachment 96930 [details]
proposed patch

[Raphael Kubo da Costa (:rakuco)]

> 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

[Eric Seidel (no email)]

Comment on attachment 96930 [details]
proposed patch

rs=me.

[Eric Seidel (no email)]

Comment on attachment 96930 [details]
proposed patch

Looks like you had a real review, they just didn't set the r-.

[Grzegorz Czajkowski]

Created attachment 97078 [details]
updated patch according to Raphael's suggestions

[Grzegorz Czajkowski]

(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

[Grzegorz Czajkowski]

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

[Raphael Kubo da Costa (:rakuco)]

Informal r+ from my side.

[Antonio Gomes]

Comment on attachment 97080 [details]
updated patch

CQ+ it when it gets API reviewed, please.

[Raphael Kubo da Costa (:rakuco)]

I've reviewed it a few days ago, but can't cq+ it.

[Lucas De Marchi]

(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.

[WebKit Review Bot]

Comment on attachment 97080 [details]
updated patch

Clearing flags on attachment: 97080

Committed r88947: <http://trac.webkit.org/changeset/88947>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.