Bug 44417 - Added WebCore::DocumentMarkerController API which returns vector describing all markers.
Summary: Added WebCore::DocumentMarkerController API which returns vector describing a...
Status: RESOLVED WONTFIX
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 44258
  Show dependency treegraph
 
Reported: 2010-08-23 05:31 PDT by Kamil Blank
Modified: 2011-05-23 13:42 PDT (History)
9 users (show)

See Also:


Attachments
patch (3.11 KB, patch)
2010-08-23 05:32 PDT, Kamil Blank
abarth: review-
Details | Formatted Diff | Diff
patch (3.19 KB, patch)
2011-02-12 00:31 PST, Kamil Blank
no flags Details | Formatted Diff | Diff
patch (4.54 KB, patch)
2011-02-12 14:29 PST, Kamil Blank
no flags Details | Formatted Diff | Diff
patch (4.53 KB, patch)
2011-02-13 00:19 PST, Kamil Blank
eric: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kamil Blank 2010-08-23 05:31:12 PDT
I need functionality which returns the position of n-th marker so I could be able to show it in browser.

First of all I needed WebCore::DocumentMarkerController functionality which returns a vector of IntRects describing text matches markers.
Unfortunately Webcore::DocumentMarkerController::renderedRectsForMarkers() returns only rects in the visible part of document.
I added new functionality WebCore::DocumentMarkerController::firstRectsForMarkers() which returns needed vector. 
Prefix "first" means that function only return first IntRect for every marker.
Comment 1 Kamil Blank 2010-08-23 05:32:09 PDT
Created attachment 65108 [details]
patch
Comment 2 Rafael Antognolli 2010-08-31 09:13:43 PDT
It seems that this functionality got in with the name DocumentMarkerController::renderedRectsForMarkers in the revision http://trac.webkit.org/changeset/65787. Isn't it the same?

Anyway, the bug 44258 was applied (even without this dependency met), and later a bugfix was done for it in bug 44929. Do you think it's still missing some functionality?
Comment 3 Rafael Antognolli 2010-08-31 09:39:35 PDT
(In reply to comment #2)
> It seems that this functionality got in with the name DocumentMarkerController::renderedRectsForMarkers in the revision http://trac.webkit.org/changeset/65787. Isn't it the same?
> 
> Anyway, the bug 44258 was applied (even without this dependency met), and later a bugfix was done for it in bug 44929. Do you think it's still missing some functionality?

Oops, I just read your first comment again. Sorry, my mistake, please don't consider the previous comment as valid.
Comment 4 Eric Seidel (no email) 2010-09-26 10:21:23 PDT
I don't know much about markers, but lots of folks have worked on them over the years.
Comment 5 Adam Barth 2010-12-23 00:15:58 PST
Comment on attachment 65108 [details]
patch

This appears to be dead code.  Is there an EFL API to go with this patch?
Comment 6 Kamil Blank 2010-12-28 22:15:42 PST
Hi Adam,

I was working on functionality which allows WebKit-EFL to mark n-th searched text on page.
There are 2 parts of my work:
1) This patch, involves in WebCore part
2) WebKit-EFL patch which uses functionality from WebCore

As you can see in Blocks section, this patch functionality is used by WebKit-EFL patch:
https://bugs.webkit.org/show_bug.cgi?id=44258

Unfortunately, due to strange (for me) behaviour the second patch landed in repository when WebCore part didn't. Because of natural build-break in this situation EFL patch has been changed so it doesn't cause any issues. Whenever this patch is landed, EFL part will be able to use WebCore functionality again.
Comment 7 Kamil Blank 2011-02-12 00:31:07 PST
Created attachment 82232 [details]
patch

patch adjusted to new sources
Comment 8 Kamil Blank 2011-02-12 11:32:57 PST
Could someone look at this patch, please?
Comment 9 Adam Barth 2011-02-12 14:04:19 PST
This patch still appears to be dead code.  Can you wire it up to the EFL API in the same patch so we can see what how this code will be used?  Ideally, we'd test the code at the same time.
Comment 10 Kamil Blank 2011-02-12 14:29:31 PST
Created attachment 82241 [details]
patch

added fix for ewk_frame_text_matches_nth_pos_get()
Comment 11 Kamil Blank 2011-02-12 14:48:12 PST
I added fix for EFL API.

It's just a simple revert of this commit: 3ea24e48e37734f9d04599963c06dd491e916a4d
git-svn-id: http://svn.webkit.org/repository/webkit/trunk@66488 268f45cc-cd09-0410-ab3c-d52691b4dbfc
Comment 12 Antonio Gomes 2011-02-12 21:45:29 PST
Comment on attachment 82232 [details]
patch

Comments start with capital letter and end with a period (.)
Comment 13 Antonio Gomes 2011-02-12 21:47:39 PST
Comment on attachment 82241 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=82241&action=review

> Source/WebCore/dom/DocumentMarkerController.cpp:342
> +            WebCore::RenderText* text = static_cast<WebCore::RenderText*>(node->renderer());

You probably do not need "WebCore::"
Comment 14 Kamil Blank 2011-02-13 00:19:31 PST
Created attachment 82253 [details]
patch

Modified regarding to Antonio comments:
+ removed WebCore:: prefix 
+ comments style
Comment 15 Kamil Blank 2011-02-18 12:19:52 PST
Hi Guys,

Are there any chances for review?
Comment 16 Kamil Blank 2011-04-19 01:51:19 PDT
Could you please take a look into my patch again, please?
Comment 17 Eric Seidel (no email) 2011-04-19 07:25:28 PDT
Comment on attachment 82253 [details]
patch

What is this for, and why is it different from renderedRects?
Comment 18 Kamil Blank 2011-04-21 06:09:25 PDT
(In reply to comment #17)
> (From update of attachment 82253 [details])
> What is this for, and why is it different from renderedRects?

It's needed to be used by below patch:
https://bugs.webkit.org/show_bug.cgi?id=44258 - due to missing function from this patch currently EFL port uses renderedRectsForMarkers() but should used firstRectsForMarkers() (which is implemented here).

The difference between both functions is that renderedRectsForMarkers() returns only rects in visible part of document but I need all rects from document to be able to return n-th marker.
Comment 19 Alexey Proskuryakov 2011-04-21 09:11:22 PDT
Bug 44258 is marked as fixed, so it's still unclear what this is needed for. The question is what is the user observable that needs to be fixed, and why other ports get away without this function today.
Comment 20 Kamil Blank 2011-04-22 00:10:51 PDT
(In reply to comment #19)
> Bug 44258 is marked as fixed, so it's still unclear what this is needed for. The question is what is the user observable that needs to be fixed, and why other ports get away without this function today.

Dear Alexey,

Please, look below at my previous comment (#6) to this bug - it's an explanation of whole situation:

> I was working on functionality which allows WebKit-EFL to mark n-th searched text on page.
> There are 2 parts of my work:
> 1) This patch, involves in WebCore part
> 2) WebKit-EFL patch which uses functionality from WebCore
> 
> As you can see in Blocks section, this patch functionality is used by WebKit-EFL patch:
> https://bugs.webkit.org/show_bug.cgi?id=44258
> 
> Unfortunately, due to strange (for me) behaviour the second patch landed in repository when WebCore part didn't. Because of natural build-break in this situation EFL patch has been changed so it doesn't cause any issues. Whenever this patch is landed, EFL part will be able to use WebCore functionality again.
Comment 21 Alexey Proskuryakov 2011-04-22 01:11:27 PDT
> I was working on functionality which allows WebKit-EFL to mark n-th searched text on page

Safari does that. Why are any WebCore changes needed when other WebKit based browsers can already do what you need?
Comment 22 Eric Seidel (no email) 2011-05-23 13:41:25 PDT
Comment on attachment 82253 [details]
patch

I believe ethis is not needed.
Comment 23 Eric Seidel (no email) 2011-05-23 13:42:17 PDT
I believe this is not needed.  Please re-open with explanation if it can be shown that the methods the other ports use are insufficient for EFLs needs.