Bug 67114

Created attachment 105472 [details] proposed patch

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?

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

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

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

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?

(In reply to comment #6) > What is that needed for? I mean is there an actual use case? Please see comments #2 and #3.

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

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

Created attachment 107859 [details] Patch

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

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.

Created attachment 109115 [details] Follow rniwa's suggestions

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

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

I file a Bug 69073 for one-letter variable.

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

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

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.

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.

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>

All reviewed patches have been landed. Closing bug.