| Summary: | [EFL] Fix and update doxygen documentation for ewk_frame | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Grzegorz Czajkowski <g.czajkowski> | ||||||||
| Component: | WebKit EFL | Assignee: | Nobody <webkit-unassigned> | ||||||||
| Status: | RESOLVED FIXED | ||||||||||
| Severity: | Minor | CC: | gyuyoung.kim, kenneth, leandro, lucas.de.marchi, rakuco, tonikitoo, webkit.review.bot | ||||||||
| Priority: | P2 | ||||||||||
| Version: | 528+ (Nightly build) | ||||||||||
| Hardware: | All | ||||||||||
| OS: | All | ||||||||||
| Attachments: |
|
||||||||||
|
Description
Grzegorz Czajkowski
2011-06-13 00:07:53 PDT
Created attachment 96930 [details]
proposed patch
> 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 on attachment 96930 [details]
proposed patch
rs=me.
Comment on attachment 96930 [details]
proposed patch
Looks like you had a real review, they just didn't set the r-.
Created attachment 97078 [details]
updated patch according to Raphael's suggestions
(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 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 Informal r+ from my side. Comment on attachment 97080 [details]
updated patch
CQ+ it when it gets API reviewed, please.
I've reviewed it a few days ago, but can't cq+ it. (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 on attachment 97080 [details] updated patch Clearing flags on attachment: 97080 Committed r88947: <http://trac.webkit.org/changeset/88947> All reviewed patches have been landed. Closing bug. |