Bug 67114 – [EFL] API to get plain text of the frame

• Component: WebKit EFL
• Status: RESOLVED FIXED

[Grzegorz Czajkowski]

This is an alternative API to ewk_frame_source_get. It gets plain text of the document without any HTML's tokens.

[Grzegorz Czajkowski]

Created attachment 105472 [details]
proposed patch

[Raphael Kubo da Costa (:rakuco)]

May I ask what are the use cases for exposing this API to the user? I don't see other ports calling outerText, and those who call innerText usually do that for DumpRenderTree only.

One other thing is that bug 66876 adds a similar private function (ewk_frame_inner_text_get); would that solve your needs if it was not private?

[Grzegorz Czajkowski]

(In reply to comment #2)
> May I ask what are the use cases for exposing this API to the user? I don't see other ports calling outerText, and those who call innerText usually do that for DumpRenderTree only.

It's needed to save HTML Pages by application like email client as plain text.

> 
> One other thing is that bug 66876 adds a similar private function (ewk_frame_inner_text_get); would that solve your needs if it was not private?

I agree that these functions are similar but implementation is a little different.

[Raphael Kubo da Costa (:rakuco)]

(In reply to comment #3)
> (In reply to comment #2)
> > One other thing is that bug 66876 adds a similar private function (ewk_frame_inner_text_get); would that solve your needs if it was not private?
> 
> I agree that these functions are similar but implementation is a little different.

The only noteworthy difference I see is that bug 66876 calls innerText() for the whole document, whereas you only call it for the body. Do the results differ too much? The function (either mine or yours) could accept a parameter specifying whether document or body should be used, if that's the case.

[Grzegorz Czajkowski]

(In reply to comment #4)
> (In reply to comment #3)
> > (In reply to comment #2)
> > > One other thing is that bug 66876 adds a similar private function (ewk_frame_inner_text_get); would that solve your needs if it was not private?
> > 
> > I agree that these functions are similar but implementation is a little different.
> 
> The only noteworthy difference I see is that bug 66876 calls innerText() for the whole document, whereas you only call it for the body. Do the results differ too much? The function (either mine or yours) could accept a parameter specifying whether document or body should be used, if that's the case.

Hi Kubo,
Let me perform some tests with ewk_frame_inner_text_get() and I will leave a feedback if we really need body param in your implementation.

[Kenneth Rohde Christiansen]

Comment on attachment 105472 [details]
proposed patch

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

> Source/WebKit/efl/ChangeLog:9
> +        This is an alternative API to ewk_frame_source_get. It gets plain text
> +        of the frame without any HTML's tokens.

What is that needed for? I mean is there an actual use case?

[Raphael Kubo da Costa (:rakuco)]

(In reply to comment #6)
> What is that needed for? I mean is there an actual use case?

Please see comments #2 and #3.

[Grzegorz Czajkowski]

(In reply to comment #5)
> (In reply to comment #4)
> > (In reply to comment #3)
> > > (In reply to comment #2)
> > > > One other thing is that bug 66876 adds a similar private function (ewk_frame_inner_text_get); would that solve your needs if it was not private?
> > > 
> > > I agree that these functions are similar but implementation is a little different.
> > 
> > The only noteworthy difference I see is that bug 66876 calls innerText() for the whole document, whereas you only call it for the body. Do the results differ too much? The function (either mine or yours) could accept a parameter specifying whether document or body should be used, if that's the case.
> 
> Hi Kubo,
> Let me perform some tests with ewk_frame_inner_text_get() and I will leave a feedback if we really need body param in your implementation.

Sorry for my late answer.
There is no any difference between results of ewk_frame_inner_text_get and ewk_frame_plain_text_get. You can make your function as public and then this bug should be marked as duplicated.

[Raphael Kubo da Costa (:rakuco)]

(In reply to comment #8)
> There is no any difference between results of ewk_frame_inner_text_get and ewk_frame_plain_text_get. You can make your function as public and then this bug should be marked as duplicated.

Due to the course of the recent events (the other patch being rejected with the suggestion that a DumpRenderTreeSupportEfl should be implemented), I think it's now better to use this bug for this specific feature. I'll work on a marge of both of our patches and submit it here for review. For now, I'll clear the flags for the current patch.

[Raphael Kubo da Costa (:rakuco)]

Created attachment 107859 [details]
Patch

[Raphael Kubo da Costa (:rakuco)]

(In reply to comment #10)
> Created an attachment (id=107859) [details]

FWIW, this newer version puts the '*' in pointers on the left side again, as being discussed in bug 68231.

[Ryosuke Niwa]

Comment on attachment 107859 [details]
Patch

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

> Source/WebKit/efl/ewk/ewk_frame.cpp:1590
> +char* ewk_frame_plain_text_get(const Evas_Object* o)

Please don't use one-letter variable.

> Source/WebKit/efl/ewk/ewk_frame.cpp:1596
> +    if (sd->frame->view() && sd->frame->view()->layoutPending())
> +        sd->frame->view()->layout();

innerText automatically calls updateLayoutIgnorePendingStylesheets.

> Source/WebKit/efl/ewk/ewk_frame.cpp:1598
> +    WebCore::Element* documentElement = sd->frame->document()->documentElement();

I don't think we should assume that the frame has a document.

[Raphael Kubo da Costa (:rakuco)]

Created attachment 109115 [details]
Follow rniwa's suggestions

[Gyuyoung Kim]

Comment on attachment 109115 [details]
Follow rniwa's suggestions

Attachment 109115 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/9898030

[Gyuyoung Kim]

(In reply to comment #12)
> (From update of attachment 107859 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=107859&action=review
> 
> > Source/WebKit/efl/ewk/ewk_frame.cpp:1590
> > +char* ewk_frame_plain_text_get(const Evas_Object* o)
> 
> Please don't use one-letter variable.

Hello Ryosuke,

one-letter variable is efl coding style. However, we are trying to be compliant with webkit coding style in Bug 68209. This one-letter variable is also one of discussing items. I'm going to file a bug for this one-letter variable, then I would like to fix all one-letter variable at a time using a script(e.g uncrustify) after discussing efl contributors enough. So, could you accept one-letter variable in this patch for now ?

[Gyuyoung Kim]

I file a Bug 69073 for one-letter variable.

[Raphael Kubo da Costa (:rakuco)]

Created attachment 109158 [details]
Follow rniwa's suggestions and make it compile

[Raphael Kubo da Costa (:rakuco)]

(In reply to comment #15)
> Hello Ryosuke,

He's not on the CC list ;)

> So, could you accept one-letter variable in this patch for now ?

I think the way to go here is whatever makes the reviewer happier -- even if this case of a one-letter variable is fixed here, uncrustify wouldn't break because of that.

[Ryosuke Niwa]

Comment on attachment 109158 [details]
Follow rniwa's suggestions and make it compile

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

> Source/WebKit/efl/ewk/ewk_frame.cpp:1592
> +    EWK_FRAME_SD_GET_OR_RETURN(ewkFrame, sd, 0);

sd? Is this a commonly-used / well-known term?

> Source/WebKit/efl/ewk/ewk_frame.cpp:1595
> +    if (!sd->frame->document())

You can probably cache Document* here.

[Ryosuke Niwa]

Comment on attachment 109158 [details]
Follow rniwa's suggestions and make it compile

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

>> Source/WebKit/efl/ewk/ewk_frame.cpp:1592
>> +    EWK_FRAME_SD_GET_OR_RETURN(ewkFrame, sd, 0);
> 
> sd? Is this a commonly-used / well-known term?

Apparently it is.

[WebKit Review Bot]

Comment on attachment 109158 [details]
Follow rniwa's suggestions and make it compile

Clearing flags on attachment: 109158

Committed r96362: <http://trac.webkit.org/changeset/96362>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.