Summary: | [EFL] Webkit-EFL API which returns position of n-th text matches mark. | ||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Kamil Blank <k.blank> | ||||||||||||||||
Component: | WebKit EFL | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||
Severity: | Normal | CC: | antognolli+webkit, commit-queue, gyuyoung.kim, leandro, lucas.de.marchi, tonikitoo, webkit.review.bot | ||||||||||||||||
Priority: | P2 | ||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||
Hardware: | PC | ||||||||||||||||||
OS: | Linux | ||||||||||||||||||
Bug Depends on: | 44417 | ||||||||||||||||||
Bug Blocks: | |||||||||||||||||||
Attachments: |
|
Description
Kamil Blank
2010-08-19 07:02:42 PDT
Created attachment 64848 [details]
patch
(In reply to comment #1) > Created an attachment (id=64848) [details] > patch Hello Kamil , Please request review and cq for this patch. Attachment 64848 [details] did not pass style-queue:
Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebKit/efl/ewk/ewk_frame.cpp:56: Alphabetical sorting problem. [build/include_order] [4]
Total errors found: 1 in 6 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Hello Kamil, You can check if your patch has any style errors by "WebKitTools/Scripts/check-webkit-style" or "WebKitTools/Scripts/webkit-patch" scripts. Current ewk_frame.cpp file has style errors as below, gyuyoung@gyuyoung-desktop:~/webkit/WebKit-EFL$ WebKitTools/Scripts/check-webkit-style WebKit/efl/ewk/ewk_frame.cpp WebKit/efl/ewk/ewk_frame.cpp:48: Alphabetical sorting problem. [build/include_order] [4] WebKit/efl/ewk/ewk_frame.cpp:52: Alphabetical sorting problem. [build/include_order] [4] Total errors found: 2 in 1 files If you can solve this errors, I think it is better to fix the style errors together. Thanks, Gyuyoung Kim Created attachment 64929 [details]
patch2
Removed style errors from ewk_frame.cpp
Hi Kamil. I just did a quick look over the WebCore::Document part, and didn't see any problem, but I think some reviewer that understands deeply that part of code should take a look at it. So maybe you should find who recently contributed to it and add to cc too. I'm not sure, but I also think that this should be split into two patches. One adding the Document::firstRectsForMarkers, and another one using this functionality inside WebKit-EFL. And please, pay attention to the coding style in ewk_frame.cpp. It does use WebKit coding style (as all the c++ files inside ewk/). So the pointers should be declared as: Evas_Object* o; instead of Evas_Object *o; Regards, Rafael Created attachment 65079 [details]
WebCore::Document patch
Created attachment 65080 [details]
WebKit-EFL patch
Created attachment 65094 [details]
WebCore:DocumentMarkerController patch
Due to last changes in WebCore::Document I had to move my changes from Document to DocumentMarkerController.
Created attachment 65095 [details]
WebKit-EFL patch
I am not sure if two patches can be reviewed in a bug. In my opinion, it seems that DOM patch needs to be reviewed by other bug. I've created new bug containing changes in WebCore::DocumentMarkerController https://bugs.webkit.org/show_bug.cgi?id=44417 Comment on attachment 65094 [details] WebCore:DocumentMarkerController patch Patch moved to new bug: https://bugs.webkit.org/show_bug.cgi?id=44417 Comment on attachment 65095 [details] WebKit-EFL patch I'm not a reviewer, just an informal review: > diff --git a/WebKit/efl/ewk/ewk_frame.cpp b/WebKit/efl/ewk/ewk_frame.cpp > index 7a2af5a..a52ed19 100644 > --- a/WebKit/efl/ewk/ewk_frame.cpp > +++ b/WebKit/efl/ewk/ewk_frame.cpp [..] > +/** > + * Comparison function used by ewk_frame_text_matches_nth_pos_get > + */ > +static bool compIntRect(WebCore::IntRect i, WebCore::IntRect j) This name is not good for 2 reasons: 1) It's meaningless. You are comparing rects for what? Give a name that tells what this function does. Since it's used by std::sort 2) Naming convention in this file is a bit different. We do not use CamelCase. My suggestion is: _ewk_frame_rect_cmp_less_than About the arguments, you are missing the consts and you are passing them in stack when you could just use references. So, this is the signature of this function I'd like to see: static bool _ewk_frame_rect_cmp_less_than(const WebCore::IntRect& i, const WebCore::IntRect& j) > +{ > + return (i.y() < j.y() || (i.y() == j.y() && i.x() < j.x())); > +} > + > +/** > + * Predicate used by ewk_frame_text_matches_nth_pos_get > + */ > +static bool isNegativeValue(WebCore::IntRect i) Same comment above for this one. > +{ [...] > +/** > + * Get x, y position of n-th text match in frame > + * > + * @param o frame object where matches are highlighted. > + * @param n index of element > + * @param x where to return x position > + * @param y where to return y position > + * > + * @return @c EINA_TRUE on success, @c EINA_FALSE for failure - when no matches found or > + * n bigger than search results. > + */ > +Eina_Bool ewk_frame_text_matches_nth_pos_get(Evas_Object* o, int n, int* x, int* y) > +{ > + EWK_FRAME_SD_GET_OR_RETURN(o, sd, EINA_FALSE); > + EINA_SAFETY_ON_NULL_RETURN_VAL(sd->frame, EINA_FALSE); In exported function you must check all incoming pointers if you are going to modify their content. So: EINA_SAFETY_ON_NULL_RETURN_VAL(x, EINA_FALSE); EINA_SAFETY_ON_NULL_RETURN_VAL(y, EINA_FALSE); > + > + Vector<WebCore::IntRect> intRects = sd->frame->document()->markers()->firstRectsForMarkers(WebCore::DocumentMarker::TextMatch); > + > + /* remove useless values */ > + std::remove_if(intRects.begin(), intRects.end(), isNegativeValue); > + > + if (intRects.isEmpty() || n > intRects.size()) > + return EINA_FALSE; > + > + std::sort(intRects.begin(), intRects.end(), compIntRect); > + > + *x = intRects[n - 1].x(); Otherwise, you could receive a segv here. > + *y = intRects[n - 1].y(); and here. (In reply to comment #14) > In exported function you must check all incoming pointers if you are going to modify their content. So: > EINA_SAFETY_ON_NULL_RETURN_VAL(x, EINA_FALSE); > EINA_SAFETY_ON_NULL_RETURN_VAL(y, EINA_FALSE); > > > + > > + Vector<WebCore::IntRect> intRects = sd->frame->document()->markers()->firstRectsForMarkers(WebCore::DocumentMarker::TextMatch); > > + > > + /* remove useless values */ > > + std::remove_if(intRects.begin(), intRects.end(), isNegativeValue); > > + > > + if (intRects.isEmpty() || n > intRects.size()) > > + return EINA_FALSE; > > + > > + std::sort(intRects.begin(), intRects.end(), compIntRect); > > + > > + *x = intRects[n - 1].x(); > Otherwise, you could receive a segv here. > > > + *y = intRects[n - 1].y(); > and here. Alternatively, you can check if the pointer is not NULL before assigning to it. This would make the parameters not obligatory: if (x) *x = intRects[n - 1].x(); Same for y. Created attachment 65228 [details]
patch
Modified regarding to Lucas and Rafael comments.
(In reply to comment #16) > Created an attachment (id=65228) [details] > patch > > Modified regarding to Lucas and Rafael comments. lgtm, although I'm wondering if this shouldn't go to WebCore so other platforms benefit from it. Antonio, what do you think? Comment on attachment 65228 [details] patch > WebKit/efl/ewk/ewk_frame.cpp:830 > +static bool _ewk_frame_rect_cmp_less_than(const WebCore::IntRect& i, const WebCore::IntRect& j) > +{ > + return (i.y() < j.y() || (i.y() == j.y() && i.x() < j.x())); > +} We generally frown upon one letter variable names. Comment on attachment 65228 [details] patch Clearing flags on attachment: 65228 Committed r66454: <http://trac.webkit.org/changeset/66454> All reviewed patches have been landed. Closing bug. (In reply to comment #20) > All reviewed patches have been landed. Closing bug. Does not it broke the EFL build since the WebCore part of the patch/feature did not land yet? (In reply to comment #21) > (In reply to comment #20) > > All reviewed patches have been landed. Closing bug. > > Does not it broke the EFL build since the WebCore part of the patch/feature did not land yet? Yes, it was broken, but the fix in http://trac.webkit.org/changeset/66488 fixes the build. I'm looking at the implementation right now, but it seems that the functionality in both functions is almost the same. |