Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
342143004:
Defect 248426: Speak rendered text when no selection is made on Mac (Closed)
DescriptionDefect 248426: Speak rendered text when no selection is made on Mac.
For more info : http://crbug.com/248426
If there is no selection on a page, use TTS to speak all the text by default.
The fix works in the following way.
An async IPC message from render_widget_host_view_mac when there is no selection to get data from WebLocalFrame::getContentAsText.
The callback for the IPC then creates a TTS session for speaking text.
The test for this defect can be followed at
https://codereview.chromium.org/464613003/
BUG=248426
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=289799
Patch Set 1 #
Total comments: 9
Patch Set 2 : Using WebLocalFrame::contentAsText() for fetching rendered text #
Total comments: 14
Patch Set 3 : Added browser tests for Defect 248426 #Patch Set 4 : Removed empty lines from render_view_browsertest_mac.mm #
Total comments: 12
Patch Set 5 : Style changes based on review comments for patch set 4 #Patch Set 6 : Reverted render_view_impl IPC signatures to its original state #
Total comments: 5
Patch Set 7 : Style changes fixed for Defect 248426 #
Total comments: 3
Patch Set 8 : Uploading a clean patch #
Total comments: 10
Patch Set 9 : Added a link to newly created defect that addresses IPC data handling capabilities #
Total comments: 6
Patch Set 10 : Shortenned comments in View_Messages.h based on reviewers feedback #Patch Set 11 : Changed message size from Renderer to Browser for a TTS session to something resonable ~8.5 mb. #
Total comments: 2
Patch Set 12 : Made chunk size for TTS more readable #
Total comments: 4
Patch Set 13 : Fixed Nit #Patch Set 14 : Removed browser tests from Render View #
Total comments: 2
Patch Set 15 : Made C++ private method to dispatch TTS session #
Total comments: 1
Patch Set 16 : Fixed Nit based on comments #
Total comments: 1
Patch Set 17 : Added a comment for TTS handler in render_widget_host_view_mac #
Messages
Total messages: 81 (0 generated)
Please start reviewing. Thanks
Thanks for working on this! Do you know if there's a person on the Blink side who's familiar with the smart clip part to also help review? (As I'm personally unfamiliar with it.) https://codereview.chromium.org/342143004/diff/1/content/browser/renderer_hos... File content/browser/renderer_host/render_widget_host_view_mac.mm (left): https://codereview.chromium.org/342143004/diff/1/content/browser/renderer_hos... content/browser/renderer_host/render_widget_host_view_mac.mm:2273: Nit: Remove superflous change https://codereview.chromium.org/342143004/diff/1/content/browser/renderer_hos... File content/browser/renderer_host/render_widget_host_view_mac.mm (right): https://codereview.chromium.org/342143004/diff/1/content/browser/renderer_hos... content/browser/renderer_host/render_widget_host_view_mac.mm:1058: if (!selected_text_.size()) { empty() https://codereview.chromium.org/342143004/diff/1/content/browser/renderer_hos... content/browser/renderer_host/render_widget_host_view_mac.mm:2059: if (text.size()) { !empty() https://codereview.chromium.org/342143004/diff/1/content/browser/renderer_hos... content/browser/renderer_host/render_widget_host_view_mac.mm:3917: Nit: Remove superflous change https://codereview.chromium.org/342143004/diff/1/content/common/view_messages.h File content/common/view_messages.h (right): https://codereview.chromium.org/342143004/diff/1/content/common/view_messages... content/common/view_messages.h:901: Nit: Extra whitespace here https://codereview.chromium.org/342143004/diff/1/content/renderer/render_view... File content/renderer/render_view_impl.cc (right): https://codereview.chromium.org/342143004/diff/1/content/renderer/render_view... content/renderer/render_view_impl.cc:1038: if (webview()) { Nit: Reverse cond and early return instead. https://codereview.chromium.org/342143004/diff/1/content/renderer/render_view... content/renderer/render_view_impl.cc:1039: std::string text = webview()->getSmartClipData(rect).utf8(); Is it possible to iterate on the chars without converting to utf8 first to find chars_to_skip? https://codereview.chromium.org/342143004/diff/1/content/renderer/render_view... content/renderer/render_view_impl.cc:1041: // of Text along with its position Nit: Missing punctuation at the end. Also, the comment does not explain the reason we're doing this. https://codereview.chromium.org/342143004/diff/1/content/renderer/render_view... content/renderer/render_view_impl.cc:1042: if (text.size()) { Nit: !text.empty(), same below.
On 2014/06/19 14:55:10, Alexei Svitkine wrote: > Thanks for working on this! > > Do you know if there's a person on the Blink side who's familiar with the smart > clip part to also help review? (As I'm personally unfamiliar with it.) I would take a look the AUTHORS list if there are any reviewers I could include. Thanks > > https://codereview.chromium.org/342143004/diff/1/content/browser/renderer_hos... > File content/browser/renderer_host/render_widget_host_view_mac.mm (left): > > https://codereview.chromium.org/342143004/diff/1/content/browser/renderer_hos... > content/browser/renderer_host/render_widget_host_view_mac.mm:2273: > Nit: Remove superflous change > > https://codereview.chromium.org/342143004/diff/1/content/browser/renderer_hos... > File content/browser/renderer_host/render_widget_host_view_mac.mm (right): > > https://codereview.chromium.org/342143004/diff/1/content/browser/renderer_hos... > content/browser/renderer_host/render_widget_host_view_mac.mm:1058: if > (!selected_text_.size()) { > empty() > > https://codereview.chromium.org/342143004/diff/1/content/browser/renderer_hos... > content/browser/renderer_host/render_widget_host_view_mac.mm:2059: if > (text.size()) { > !empty() > > https://codereview.chromium.org/342143004/diff/1/content/browser/renderer_hos... > content/browser/renderer_host/render_widget_host_view_mac.mm:3917: > Nit: Remove superflous change > > https://codereview.chromium.org/342143004/diff/1/content/common/view_messages.h > File content/common/view_messages.h (right): > > https://codereview.chromium.org/342143004/diff/1/content/common/view_messages... > content/common/view_messages.h:901: > Nit: Extra whitespace here > > https://codereview.chromium.org/342143004/diff/1/content/renderer/render_view... > File content/renderer/render_view_impl.cc (right): > > https://codereview.chromium.org/342143004/diff/1/content/renderer/render_view... > content/renderer/render_view_impl.cc:1038: if (webview()) { > Nit: Reverse cond and early return instead. > > https://codereview.chromium.org/342143004/diff/1/content/renderer/render_view... > content/renderer/render_view_impl.cc:1039: std::string text = > webview()->getSmartClipData(rect).utf8(); > Is it possible to iterate on the chars without converting to utf8 first to find > chars_to_skip? > > https://codereview.chromium.org/342143004/diff/1/content/renderer/render_view... > content/renderer/render_view_impl.cc:1041: // of Text along with its position > Nit: Missing punctuation at the end. > > Also, the comment does not explain the reason we're doing this. > > https://codereview.chromium.org/342143004/diff/1/content/renderer/render_view... > content/renderer/render_view_impl.cc:1042: if (text.size()) { > Nit: !text.empty(), same below.
The CQ bit was checked by a.sarkar.arun@gmail.com
The CQ bit was unchecked by a.sarkar.arun@gmail.com
On 2014/06/19 15:30:14, eka508 wrote: > The CQ bit was unchecked by mailto:a.sarkar.arun@gmail.com Apparently changes have been made to WebCore::SmartClip.cpp related to CL https://codereview.chromium.org/259263003 I will make changes and submit an updated patch. Thanks
Removed use of getSmartClipForRect since it returned text for current focussed frame, thats not something we'd want here. We need something identical to text returned from a SelectAll call. The patch resolves this using WebLocalFrame::contentAsText. Please start the review. Thanks
Can you add a browser test for this? https://codereview.chromium.org/342143004/diff/20001/content/browser/renderer... File content/browser/renderer_host/render_widget_host_view_mac.mm (left): https://codereview.chromium.org/342143004/diff/20001/content/browser/renderer... content/browser/renderer_host/render_widget_host_view_mac.mm:2282: Nit: Revert unnecessary whitespace change. https://codereview.chromium.org/342143004/diff/20001/content/browser/renderer... File content/browser/renderer_host/render_widget_host_view_mac.mm (right): https://codereview.chromium.org/342143004/diff/20001/content/browser/renderer... content/browser/renderer_host/render_widget_host_view_mac.mm:637: OnGetRenderedTextCompleted) Nit: Align. https://codereview.chromium.org/342143004/diff/20001/content/browser/renderer... content/browser/renderer_host/render_widget_host_view_mac.mm:1065: if (render_widget_host_) { Nit: Combine with previous if. https://codereview.chromium.org/342143004/diff/20001/content/browser/renderer... content/browser/renderer_host/render_widget_host_view_mac.mm:1082: // Clear selected text when StopSpeaking is called Why? |selected_text_| is supposed to be tracking what's selected on the renderer size. So unless the selection changed there, I don't think we should be clearing it. https://codereview.chromium.org/342143004/diff/20001/content/browser/renderer... content/browser/renderer_host/render_widget_host_view_mac.mm:2066: void RenderWidgetHostViewMac::OnGetRenderedTextCompleted(const std::string& text) { Nit: This is over 80 chars, wrap at the ( https://codereview.chromium.org/342143004/diff/20001/content/browser/renderer... content/browser/renderer_host/render_widget_host_view_mac.mm:2072: [NSApp speakString:base::SysUTF8ToNSString(text)]; Nit: Indent 2 less. https://codereview.chromium.org/342143004/diff/20001/content/browser/renderer... content/browser/renderer_host/render_widget_host_view_mac.mm:2073: Nit: Remove empty line.
On 2014/06/23 14:35:16, Alexei Svitkine wrote: > Can you add a browser test for this? > Is there a proper way to test TTS currently for chrome? I was thinking to add a test against SelectAll and see if selected_text_ is same as text returned from the IPC call back for OnGetRenderedTextCompleted. Would that make sense? Thanks > https://codereview.chromium.org/342143004/diff/20001/content/browser/renderer... > File content/browser/renderer_host/render_widget_host_view_mac.mm (left): > > https://codereview.chromium.org/342143004/diff/20001/content/browser/renderer... > content/browser/renderer_host/render_widget_host_view_mac.mm:2282: > Nit: Revert unnecessary whitespace change. > > https://codereview.chromium.org/342143004/diff/20001/content/browser/renderer... > File content/browser/renderer_host/render_widget_host_view_mac.mm (right): > > https://codereview.chromium.org/342143004/diff/20001/content/browser/renderer... > content/browser/renderer_host/render_widget_host_view_mac.mm:637: > OnGetRenderedTextCompleted) > Nit: Align. > > https://codereview.chromium.org/342143004/diff/20001/content/browser/renderer... > content/browser/renderer_host/render_widget_host_view_mac.mm:1065: if > (render_widget_host_) { > Nit: Combine with previous if. > > https://codereview.chromium.org/342143004/diff/20001/content/browser/renderer... > content/browser/renderer_host/render_widget_host_view_mac.mm:1082: // Clear > selected text when StopSpeaking is called > Why? |selected_text_| is supposed to be tracking what's selected on the renderer > size. So unless the selection changed there, I don't think we should be clearing > it. > > https://codereview.chromium.org/342143004/diff/20001/content/browser/renderer... > content/browser/renderer_host/render_widget_host_view_mac.mm:2066: void > RenderWidgetHostViewMac::OnGetRenderedTextCompleted(const std::string& text) { > Nit: This is over 80 chars, wrap at the ( > > https://codereview.chromium.org/342143004/diff/20001/content/browser/renderer... > content/browser/renderer_host/render_widget_host_view_mac.mm:2072: [NSApp > speakString:base::SysUTF8ToNSString(text)]; > Nit: Indent 2 less. > > https://codereview.chromium.org/342143004/diff/20001/content/browser/renderer... > content/browser/renderer_host/render_widget_host_view_mac.mm:2073: > Nit: Remove empty line.
I agree that there's probably no good way to test TTS, but yeah testing the text string returned corresponds to what the webpage is showing is what I was thinking of. On Mon, Jun 23, 2014 at 11:04 AM, <a.sarkar.arun@gmail.com> wrote: > On 2014/06/23 14:35:16, Alexei Svitkine wrote: > >> Can you add a browser test for this? >> > > > Is there a proper way to test TTS currently for chrome? I was thinking to > add a > test against SelectAll and see if selected_text_ is same as text returned > from > the IPC call back for OnGetRenderedTextCompleted. Would that make sense? > > Thanks > > > > https://codereview.chromium.org/342143004/diff/20001/ > content/browser/renderer_host/render_widget_host_view_mac.mm > >> File content/browser/renderer_host/render_widget_host_view_mac.mm (left): >> > > > https://codereview.chromium.org/342143004/diff/20001/ > content/browser/renderer_host/render_widget_host_view_mac.mm#oldcode2282 > >> content/browser/renderer_host/render_widget_host_view_mac.mm:2282: >> Nit: Revert unnecessary whitespace change. >> > > > https://codereview.chromium.org/342143004/diff/20001/ > content/browser/renderer_host/render_widget_host_view_mac.mm > >> File content/browser/renderer_host/render_widget_host_view_mac.mm >> (right): >> > > > https://codereview.chromium.org/342143004/diff/20001/ > content/browser/renderer_host/render_widget_host_view_mac.mm#newcode637 > >> content/browser/renderer_host/render_widget_host_view_mac.mm:637: >> OnGetRenderedTextCompleted) >> Nit: Align. >> > > > https://codereview.chromium.org/342143004/diff/20001/ > content/browser/renderer_host/render_widget_host_view_mac.mm#newcode1065 > >> content/browser/renderer_host/render_widget_host_view_mac.mm:1065: if >> (render_widget_host_) { >> Nit: Combine with previous if. >> > > > https://codereview.chromium.org/342143004/diff/20001/ > content/browser/renderer_host/render_widget_host_view_mac.mm#newcode1082 > >> content/browser/renderer_host/render_widget_host_view_mac.mm:1082: // >> Clear >> selected text when StopSpeaking is called >> Why? |selected_text_| is supposed to be tracking what's selected on the >> > renderer > >> size. So unless the selection changed there, I don't think we should be >> > clearing > >> it. >> > > > https://codereview.chromium.org/342143004/diff/20001/ > content/browser/renderer_host/render_widget_host_view_mac.mm#newcode2066 > >> content/browser/renderer_host/render_widget_host_view_mac.mm:2066: void >> RenderWidgetHostViewMac::OnGetRenderedTextCompleted(const std::string& >> text) { >> Nit: This is over 80 chars, wrap at the ( >> > > > https://codereview.chromium.org/342143004/diff/20001/ > content/browser/renderer_host/render_widget_host_view_mac.mm#newcode2072 > >> content/browser/renderer_host/render_widget_host_view_mac.mm:2072: [NSApp >> speakString:base::SysUTF8ToNSString(text)]; >> Nit: Indent 2 less. >> > > > https://codereview.chromium.org/342143004/diff/20001/ > content/browser/renderer_host/render_widget_host_view_mac.mm#newcode2073 > >> content/browser/renderer_host/render_widget_host_view_mac.mm:2073: >> Nit: Remove empty line. >> > > > https://codereview.chromium.org/342143004/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2014/06/23 15:47:06, Alexei Svitkine wrote: > I agree that there's probably no good way to test TTS, but yeah testing the > text string returned corresponds to what the webpage is showing is what I > was thinking of. > Also would it make more sense to add an unittest instead of browsertest since they are faster ? > > On Mon, Jun 23, 2014 at 11:04 AM, <mailto:a.sarkar.arun@gmail.com> wrote: > > > On 2014/06/23 14:35:16, Alexei Svitkine wrote: > > > >> Can you add a browser test for this? > >> > > > > > > Is there a proper way to test TTS currently for chrome? I was thinking to > > add a > > test against SelectAll and see if selected_text_ is same as text returned > > from > > the IPC call back for OnGetRenderedTextCompleted. Would that make sense? > > > > Thanks > > > > > > > > https://codereview.chromium.org/342143004/diff/20001/ > > content/browser/renderer_host/render_widget_host_view_mac.mm > > > >> File content/browser/renderer_host/render_widget_host_view_mac.mm (left): > >> > > > > > > https://codereview.chromium.org/342143004/diff/20001/ > > content/browser/renderer_host/render_widget_host_view_mac.mm#oldcode2282 > > > >> content/browser/renderer_host/render_widget_host_view_mac.mm:2282: > >> Nit: Revert unnecessary whitespace change. > >> > > > > > > https://codereview.chromium.org/342143004/diff/20001/ > > content/browser/renderer_host/render_widget_host_view_mac.mm > > > >> File content/browser/renderer_host/render_widget_host_view_mac.mm > >> (right): > >> > > > > > > https://codereview.chromium.org/342143004/diff/20001/ > > content/browser/renderer_host/render_widget_host_view_mac.mm#newcode637 > > > >> content/browser/renderer_host/render_widget_host_view_mac.mm:637: > >> OnGetRenderedTextCompleted) > >> Nit: Align. > >> > > > > > > https://codereview.chromium.org/342143004/diff/20001/ > > content/browser/renderer_host/render_widget_host_view_mac.mm#newcode1065 > > > >> content/browser/renderer_host/render_widget_host_view_mac.mm:1065: if > >> (render_widget_host_) { > >> Nit: Combine with previous if. > >> > > > > > > https://codereview.chromium.org/342143004/diff/20001/ > > content/browser/renderer_host/render_widget_host_view_mac.mm#newcode1082 > > > >> content/browser/renderer_host/render_widget_host_view_mac.mm:1082: // > >> Clear > >> selected text when StopSpeaking is called > >> Why? |selected_text_| is supposed to be tracking what's selected on the > >> > > renderer > > > >> size. So unless the selection changed there, I don't think we should be > >> > > clearing > > > >> it. > >> > > > > > > https://codereview.chromium.org/342143004/diff/20001/ > > content/browser/renderer_host/render_widget_host_view_mac.mm#newcode2066 > > > >> content/browser/renderer_host/render_widget_host_view_mac.mm:2066: void > >> RenderWidgetHostViewMac::OnGetRenderedTextCompleted(const std::string& > >> text) { > >> Nit: This is over 80 chars, wrap at the ( > >> > > > > > > https://codereview.chromium.org/342143004/diff/20001/ > > content/browser/renderer_host/render_widget_host_view_mac.mm#newcode2072 > > > >> content/browser/renderer_host/render_widget_host_view_mac.mm:2072: [NSApp > >> speakString:base::SysUTF8ToNSString(text)]; > >> Nit: Indent 2 less. > >> > > > > > > https://codereview.chromium.org/342143004/diff/20001/ > > content/browser/renderer_host/render_widget_host_view_mac.mm#newcode2073 > > > >> content/browser/renderer_host/render_widget_host_view_mac.mm:2073: > >> Nit: Remove empty line. > >> > > > > > > https://codereview.chromium.org/342143004/ > > > > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org.
If you are able to do it in a unit test, for sure! However, I think that a browser test may be required for this, since you need to run a renderer and such, which I'm not sure is easy/possible to do in a unit test. On Mon, Jun 23, 2014 at 3:46 PM, <a.sarkar.arun@gmail.com> wrote: > On 2014/06/23 15:47:06, Alexei Svitkine wrote: > >> I agree that there's probably no good way to test TTS, but yeah testing >> the >> text string returned corresponds to what the webpage is showing is what I >> was thinking of. >> > > > Also would it make more sense to add an unittest instead of browsertest > since > they are faster ? > > > > On Mon, Jun 23, 2014 at 11:04 AM, <mailto:a.sarkar.arun@gmail.com> wrote: >> > > > On 2014/06/23 14:35:16, Alexei Svitkine wrote: >> > >> >> Can you add a browser test for this? >> >> >> > >> > >> > Is there a proper way to test TTS currently for chrome? I was thinking >> to >> > add a >> > test against SelectAll and see if selected_text_ is same as text >> returned >> > from >> > the IPC call back for OnGetRenderedTextCompleted. Would that make sense? >> > >> > Thanks >> > >> > >> > >> > https://codereview.chromium.org/342143004/diff/20001/ >> > content/browser/renderer_host/render_widget_host_view_mac.mm >> > >> >> File content/browser/renderer_host/render_widget_host_view_mac.mm >> (left): >> >> >> > >> > >> > https://codereview.chromium.org/342143004/diff/20001/ >> > content/browser/renderer_host/render_widget_host_view_mac. >> mm#oldcode2282 >> > >> >> content/browser/renderer_host/render_widget_host_view_mac.mm:2282: >> >> Nit: Revert unnecessary whitespace change. >> >> >> > >> > >> > https://codereview.chromium.org/342143004/diff/20001/ >> > content/browser/renderer_host/render_widget_host_view_mac.mm >> > >> >> File content/browser/renderer_host/render_widget_host_view_mac.mm >> >> (right): >> >> >> > >> > >> > https://codereview.chromium.org/342143004/diff/20001/ >> > content/browser/renderer_host/render_widget_host_view_mac.mm#newcode637 >> > >> >> content/browser/renderer_host/render_widget_host_view_mac.mm:637: >> >> OnGetRenderedTextCompleted) >> >> Nit: Align. >> >> >> > >> > >> > https://codereview.chromium.org/342143004/diff/20001/ >> > content/browser/renderer_host/render_widget_host_view_mac. >> mm#newcode1065 >> > >> >> content/browser/renderer_host/render_widget_host_view_mac.mm:1065: if >> >> (render_widget_host_) { >> >> Nit: Combine with previous if. >> >> >> > >> > >> > https://codereview.chromium.org/342143004/diff/20001/ >> > content/browser/renderer_host/render_widget_host_view_mac. >> mm#newcode1082 >> > >> >> content/browser/renderer_host/render_widget_host_view_mac.mm:1082: // >> >> Clear >> >> selected text when StopSpeaking is called >> >> Why? |selected_text_| is supposed to be tracking what's selected on the >> >> >> > renderer >> > >> >> size. So unless the selection changed there, I don't think we should be >> >> >> > clearing >> > >> >> it. >> >> >> > >> > >> > https://codereview.chromium.org/342143004/diff/20001/ >> > content/browser/renderer_host/render_widget_host_view_mac. >> mm#newcode2066 >> > >> >> content/browser/renderer_host/render_widget_host_view_mac.mm:2066: >> void >> >> RenderWidgetHostViewMac::OnGetRenderedTextCompleted(const std::string& >> >> text) { >> >> Nit: This is over 80 chars, wrap at the ( >> >> >> > >> > >> > https://codereview.chromium.org/342143004/diff/20001/ >> > content/browser/renderer_host/render_widget_host_view_mac. >> mm#newcode2072 >> > >> >> content/browser/renderer_host/render_widget_host_view_mac.mm:2072: >> [NSApp >> >> speakString:base::SysUTF8ToNSString(text)]; >> >> Nit: Indent 2 less. >> >> >> > >> > >> > https://codereview.chromium.org/342143004/diff/20001/ >> > content/browser/renderer_host/render_widget_host_view_mac. >> mm#newcode2073 >> > >> >> content/browser/renderer_host/render_widget_host_view_mac.mm:2073: >> >> Nit: Remove empty line. >> >> >> > >> > >> > https://codereview.chromium.org/342143004/ >> > >> > > To unsubscribe from this group and stop receiving emails from it, send an >> > email > >> to mailto:chromium-reviews+unsubscribe@chromium.org. >> > > > > https://codereview.chromium.org/342143004/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Added a browser test. The test asserts text returned from SelectAll call against WebLocalFrame::contentAsText. PTAL Thanks
PTAL https://codereview.chromium.org/342143004/diff/20001/content/browser/renderer... File content/browser/renderer_host/render_widget_host_view_mac.mm (left): https://codereview.chromium.org/342143004/diff/20001/content/browser/renderer... content/browser/renderer_host/render_widget_host_view_mac.mm:2282: On 2014/06/23 14:35:15, Alexei Svitkine wrote: > Nit: Revert unnecessary whitespace change. Done. https://codereview.chromium.org/342143004/diff/20001/content/browser/renderer... File content/browser/renderer_host/render_widget_host_view_mac.mm (right): https://codereview.chromium.org/342143004/diff/20001/content/browser/renderer... content/browser/renderer_host/render_widget_host_view_mac.mm:637: OnGetRenderedTextCompleted) On 2014/06/23 14:35:15, Alexei Svitkine wrote: > Nit: Align. Done. https://codereview.chromium.org/342143004/diff/20001/content/browser/renderer... content/browser/renderer_host/render_widget_host_view_mac.mm:1065: if (render_widget_host_) { On 2014/06/23 14:35:15, Alexei Svitkine wrote: > Nit: Combine with previous if. Done. https://codereview.chromium.org/342143004/diff/20001/content/browser/renderer... content/browser/renderer_host/render_widget_host_view_mac.mm:1082: // Clear selected text when StopSpeaking is called On 2014/06/23 14:35:16, Alexei Svitkine wrote: > Why? |selected_text_| is supposed to be tracking what's selected on the renderer > size. So unless the selection changed there, I don't think we should be clearing > it. Done. https://codereview.chromium.org/342143004/diff/20001/content/browser/renderer... content/browser/renderer_host/render_widget_host_view_mac.mm:2066: void RenderWidgetHostViewMac::OnGetRenderedTextCompleted(const std::string& text) { On 2014/06/23 14:35:15, Alexei Svitkine wrote: > Nit: This is over 80 chars, wrap at the ( Done. https://codereview.chromium.org/342143004/diff/20001/content/browser/renderer... content/browser/renderer_host/render_widget_host_view_mac.mm:2072: [NSApp speakString:base::SysUTF8ToNSString(text)]; On 2014/06/23 14:35:15, Alexei Svitkine wrote: > Nit: Indent 2 less. Done. https://codereview.chromium.org/342143004/diff/20001/content/browser/renderer... content/browser/renderer_host/render_widget_host_view_mac.mm:2073: On 2014/06/23 14:35:15, Alexei Svitkine wrote: > Nit: Remove empty line. Done.
Looks good, just mostly style comments for you. https://codereview.chromium.org/342143004/diff/60001/content/browser/renderer... File content/browser/renderer_host/render_widget_host_view_mac.mm (left): https://codereview.chromium.org/342143004/diff/60001/content/browser/renderer... content/browser/renderer_host/render_widget_host_view_mac.mm:473: Nit: Revert this whitespace change. https://codereview.chromium.org/342143004/diff/60001/content/browser/renderer... content/browser/renderer_host/render_widget_host_view_mac.mm:2282: Nit: Revert this whitespace change. https://codereview.chromium.org/342143004/diff/60001/content/browser/renderer... File content/browser/renderer_host/render_widget_host_view_mac.mm (right): https://codereview.chromium.org/342143004/diff/60001/content/browser/renderer... content/browser/renderer_host/render_widget_host_view_mac.mm:1065: // Route an IPC message to get content as text for a web contents. Nit: Indent 2 less. https://codereview.chromium.org/342143004/diff/60001/content/browser/renderer... content/browser/renderer_host/render_widget_host_view_mac.mm:1070: if ([NSApp respondsToSelector:@selector(speakString:)]) Nit: Do an early return at the top of the function if this is false. Then, you don't need to check it in OnGetRenderedTextCompleted, since it's guaranteed to be true in that case. https://codereview.chromium.org/342143004/diff/60001/content/browser/renderer... File content/browser/renderer_host/render_widget_host_view_mac_unittest.mm (right): https://codereview.chromium.org/342143004/diff/60001/content/browser/renderer... content/browser/renderer_host/render_widget_host_view_mac_unittest.mm:27: Nit: Revert this whitespace change. https://codereview.chromium.org/342143004/diff/60001/content/common/view_mess... File content/common/view_messages.h (right): https://codereview.chromium.org/342143004/diff/60001/content/common/view_mess... content/common/view_messages.h:915: IPC_MESSAGE_ROUTED1(ViewMsg_GetSmartClipDataFromRect, gfx::Rect /* rect */) Is this part of the change needed? https://codereview.chromium.org/342143004/diff/60001/content/renderer/render_... File content/renderer/render_view_browsertest_mac.mm (right): https://codereview.chromium.org/342143004/diff/60001/content/renderer/render_... content/renderer/render_view_browsertest_mac.mm:164: RenderViewImpl* view = static_cast<RenderViewImpl*>(view_); Nit: Move this closer to where it's used - i.e. right above line 170. https://codereview.chromium.org/342143004/diff/60001/content/renderer/render_... content/renderer/render_view_browsertest_mac.mm:168: base::string16 output; Nit: Move this closer to where it's used - i.e. you can declare and assign it all on one line at line 179 and same thing for line 180 for select_all_text. https://codereview.chromium.org/342143004/diff/60001/content/renderer/render_... File content/renderer/render_view_impl.cc (right): https://codereview.chromium.org/342143004/diff/60001/content/renderer/render_... content/renderer/render_view_impl.cc:1026: if (!text.empty()) Why not send it even if it's empty? You're already checking this in the receiver plus I think it would be confusing to sometimes not get a response to the IPC. https://codereview.chromium.org/342143004/diff/60001/content/renderer/render_... File content/renderer/render_view_impl.h (right): https://codereview.chromium.org/342143004/diff/60001/content/renderer/render_... content/renderer/render_view_impl.h:758: void OnExtractSmartClipData(const gfx::Rect& rect); Revert this part of the change.
Made style changes. PTAL https://codereview.chromium.org/342143004/diff/60001/content/renderer/render_... File content/renderer/render_view_impl.h (right): https://codereview.chromium.org/342143004/diff/60001/content/renderer/render_... content/renderer/render_view_impl.h:758: void OnExtractSmartClipData(const gfx::Rect& rect); On 2014/06/25 13:58:29, Alexei Svitkine wrote: > Revert this part of the change. I think Android still uses it.
https://codereview.chromium.org/342143004/diff/60001/content/renderer/render_... File content/renderer/render_view_impl.h (right): https://codereview.chromium.org/342143004/diff/60001/content/renderer/render_... content/renderer/render_view_impl.h:758: void OnExtractSmartClipData(const gfx::Rect& rect); On 2014/06/25 22:36:56, eka508 wrote: > On 2014/06/25 13:58:29, Alexei Svitkine wrote: > > Revert this part of the change. > > I think Android still uses it. > I understand that. My point is you should revert to the original version of the code. If you look at the diffs in your CL, you'll see that you're actually re-ordering the declaration of this method. Please change back this part of this change. (And anywhere else in the CL where there's a diff as a result of some previous thing you were doing with the CL but is not needed anymore.) Thanks!
On 2014/06/26 13:06:26, Alexei Svitkine wrote: > https://codereview.chromium.org/342143004/diff/60001/content/renderer/render_... > File content/renderer/render_view_impl.h (right): > > https://codereview.chromium.org/342143004/diff/60001/content/renderer/render_... > content/renderer/render_view_impl.h:758: void OnExtractSmartClipData(const > gfx::Rect& rect); > On 2014/06/25 22:36:56, eka508 wrote: > > On 2014/06/25 13:58:29, Alexei Svitkine wrote: > > > Revert this part of the change. > > > > I think Android still uses it. > > > > I understand that. My point is you should revert to the original version of the > code. If you look at the diffs in your CL, you'll see that you're actually > re-ordering the declaration of this method. Please change back this part of this > change. (And anywhere else in the CL where there's a diff as a result of some > previous thing you were doing with the CL but is not needed anymore.) Thanks! Got it. Will submit a patch later today. Thanks
I think I reverted back the changes for the most part. PTAL Thanks
https://codereview.chromium.org/342143004/diff/100001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_mac.mm (left): https://codereview.chromium.org/342143004/diff/100001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_mac.mm:491: You still have an unnecessary whitespace diff here. Please re-add the empty line here you've removed. https://codereview.chromium.org/342143004/diff/100001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_mac.mm:2324: You still have an unnecessary whitespace diff here. Please re-add the empty line here you've removed. https://codereview.chromium.org/342143004/diff/100001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_mac.mm (right): https://codereview.chromium.org/342143004/diff/100001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_mac.mm:1091: return; Indent this 2 less. https://codereview.chromium.org/342143004/diff/100001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_mac_unittest.mm (right): https://codereview.chromium.org/342143004/diff/100001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_mac_unittest.mm:27: You still have an unnecessary whitespace diff here. Please remove the empty line here you've added. https://codereview.chromium.org/342143004/diff/100001/content/common/view_mes... File content/common/view_messages.h (right): https://codereview.chromium.org/342143004/diff/100001/content/common/view_mes... content/common/view_messages.h:887: IPC_MESSAGE_ROUTED0(ViewMsg_GetRenderedText) Add a comment.
On 2014/06/27 13:01:34, Alexei Svitkine wrote: > https://codereview.chromium.org/342143004/diff/100001/content/browser/rendere... > File content/browser/renderer_host/render_widget_host_view_mac.mm (left): > > https://codereview.chromium.org/342143004/diff/100001/content/browser/rendere... > content/browser/renderer_host/render_widget_host_view_mac.mm:491: > You still have an unnecessary whitespace diff here. Please re-add the empty line > here you've removed. > > https://codereview.chromium.org/342143004/diff/100001/content/browser/rendere... > content/browser/renderer_host/render_widget_host_view_mac.mm:2324: > You still have an unnecessary whitespace diff here. Please re-add the empty line > here you've removed. > > https://codereview.chromium.org/342143004/diff/100001/content/browser/rendere... > File content/browser/renderer_host/render_widget_host_view_mac.mm (right): > > https://codereview.chromium.org/342143004/diff/100001/content/browser/rendere... > content/browser/renderer_host/render_widget_host_view_mac.mm:1091: return; > Indent this 2 less. > > https://codereview.chromium.org/342143004/diff/100001/content/browser/rendere... > File content/browser/renderer_host/render_widget_host_view_mac_unittest.mm > (right): > > https://codereview.chromium.org/342143004/diff/100001/content/browser/rendere... > content/browser/renderer_host/render_widget_host_view_mac_unittest.mm:27: > You still have an unnecessary whitespace diff here. Please remove the empty line > here you've added. > > https://codereview.chromium.org/342143004/diff/100001/content/common/view_mes... > File content/common/view_messages.h (right): > > https://codereview.chromium.org/342143004/diff/100001/content/common/view_mes... > content/common/view_messages.h:887: IPC_MESSAGE_ROUTED0(ViewMsg_GetRenderedText) > Add a comment. Done.
PTAL
lgtm % diff weirdness I noticed in a previous patchset you added yourself to AUTHORS file, but it's removed now. I don't see you in AUTHORS in TOT, so maybe you need to re-add yourself? Also, have you signed the CLA per instructions here? http://dev.chromium.org/developers/contributing-code/external-contributor-che... If not, you need to do that before landing a change. https://codereview.chromium.org/342143004/diff/120001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_mac.mm (right): https://codereview.chromium.org/342143004/diff/120001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_mac.mm:491: Hmm, I don't understand why the diff looks so. I checked the current file at TOT, and it has a blank line here, so your diff _shouldn't_ look like it's adding a line to this place, since there's one already. Can you check how you produced the diff to make sure it's diffing against master? (else your patch might not apply). https://codereview.chromium.org/342143004/diff/120001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_mac_unittest.mm (left): https://codereview.chromium.org/342143004/diff/120001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_mac_unittest.mm:27: Ditto here.
On 2014/06/27 15:54:30, Alexei Svitkine wrote: > lgtm % diff weirdness > > I noticed in a previous patchset you added yourself to AUTHORS file, but it's > removed now. I don't see you in AUTHORS in TOT, so maybe you need to re-add > yourself? > Thats strange. I already see my name in AUTHORS file > Also, have you signed the CLA per instructions here? > http://dev.chromium.org/developers/contributing-code/external-contributor-che... > > If not, you need to do that before landing a change. > > https://codereview.chromium.org/342143004/diff/120001/content/browser/rendere... > File content/browser/renderer_host/render_widget_host_view_mac.mm (right): > > https://codereview.chromium.org/342143004/diff/120001/content/browser/rendere... > content/browser/renderer_host/render_widget_host_view_mac.mm:491: > Hmm, I don't understand why the diff looks so. I checked the current file at > TOT, and it has a blank line here, so your diff _shouldn't_ look like it's > adding a line to this place, since there's one already. > > Can you check how you produced the diff to make sure it's diffing against > master? (else your patch might not apply). > > https://codereview.chromium.org/342143004/diff/120001/content/browser/rendere... > File content/browser/renderer_host/render_widget_host_view_mac_unittest.mm > (left): > > https://codereview.chromium.org/342143004/diff/120001/content/browser/rendere... > content/browser/renderer_host/render_widget_host_view_mac_unittest.mm:27: > Ditto here. I am took a diff using the command git diff master..HEAD | subl I see the changes as requested. I am little confused here. Thanks
https://codereview.chromium.org/342143004/diff/120001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_mac.mm (right): https://codereview.chromium.org/342143004/diff/120001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_mac.mm:491: On 2014/06/27 15:54:30, Alexei Svitkine wrote: > Hmm, I don't understand why the diff looks so. I checked the current file at > TOT, and it has a blank line here, so your diff _shouldn't_ look like it's > adding a line to this place, since there's one already. > > Can you check how you produced the diff to make sure it's diffing against > master? (else your patch might not apply). This is really strange. I had to throw in an extra line there since there wasn't any before i submitted the patch.
The CQ bit was checked by a.sarkar.arun@gmail.com
The CQ bit was unchecked by a.sarkar.arun@gmail.com
The CQ bit was checked by a.sarkar.arun@gmail.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/a.sarkar.arun@gmail.com/342143004/120001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: android_aosp on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/8...) android_chromium_gn_compile_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_chromium_gn_c...) android_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...) android_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/20...) chromium_presubmit on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...) ios_dbg_simulator on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...) ios_rel_device on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device/builds...) ios_rel_device_ninja on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...) linux_chromium_chromeos_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...) linux_chromium_chromeos_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...) linux_chromium_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_clang_...) linux_chromium_gn_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...) linux_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...) mac_chromium_compile_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_compile_...) mac_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...) win_chromium_compile_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...) win_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...) win_chromium_x64_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...) linux_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...) mac_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/22656)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios_dbg_simulator on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...) ios_rel_device_ninja on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...) mac_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...) mac_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/22657)
I think you need more owners to review this, before you can land. Also, please make sure you're in AUTHORS and have signed the CLA.
On 2014/06/27 20:02:27, Alexei Svitkine wrote: > I think you need more owners to review this, before you can land. Also, please > make sure you're in AUTHORS and have signed the CLA. Apparently i pushed my changes to a remote on a different machine, not sure if that caused this weirdness. Looking at the logs where the build broke it seemed like the patch only had 3 files. I uploaded a clean patch. I already have included couple of them from WebKit authors folder. Will include couple of more people
Including more reviewers PTAL Thanks
Added more reviewers. PTAL. Thanks
cevans: Could you take a look at the new IPC please? Explicitly do we have concerns about sending back strings which are too large? https://codereview.chromium.org/342143004/diff/140001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_mac.mm (right): https://codereview.chromium.org/342143004/diff/140001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_mac.mm:1089: // Route an IPC message to get content as text for a web contents. Needs better comment, how about: // If there's no selection, speak all text. In this case, send an asynchronous request for all the text in the webcontents. ViewMsg_GetRenderedTextCompleted is sent back on completion. https://codereview.chromium.org/342143004/diff/140001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_mac.mm:1095: [NSApp speakString:base::SysUTF8ToNSString(selected_text_)]; My preference would be to have this moved to a private method - e.g. SpeakText() and then call that both from here and OnGetRenderedTextCompleted(). https://codereview.chromium.org/342143004/diff/140001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_mac.mm:2119: return; Just thinking out loud - are we ok with a browser issuing multiple TTS request to a renderer when one is already in flight. i.e. I request a "speak all text" command, renderer takes a while to respond, then I do it again and I have 2 queued up. Does it make sense to drop any new TTS commands on the floor for a webview if one is already in progress? https://codereview.chromium.org/342143004/diff/140001/content/common/view_mes... File content/common/view_messages.h (right): https://codereview.chromium.org/342143004/diff/140001/content/common/view_mes... content/common/view_messages.h:887: // Notifies render view to retrieve rendered text from WebView. // Issue an async request for the full textual content of WebView, response returned by ... IPC. https://codereview.chromium.org/342143004/diff/140001/content/common/view_mes... content/common/view_messages.h:1677: // Receives content of a webpage as plain text. nit: "web page" https://codereview.chromium.org/342143004/diff/140001/content/renderer/render... File content/renderer/render_view_browsertest_mac.mm (right): https://codereview.chromium.org/342143004/diff/140001/content/renderer/render... content/renderer/render_view_browsertest_mac.mm:143: TEST_F(RenderViewTest, OnTtsForEmptySelection) { I'm sorry but I don't understand this test. Can you please explain what part of this CL the test is for?
https://codereview.chromium.org/342143004/diff/140001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_mac.mm (right): https://codereview.chromium.org/342143004/diff/140001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_mac.mm:2119: return; On 2014/06/29 11:38:55, jeremy wrote: > Just thinking out loud - are we ok with a browser issuing multiple TTS request > to a renderer when one is already in flight. > i.e. I request a "speak all text" command, renderer takes a while to respond, > then I do it again and I have 2 queued up. Does it make sense to drop any new > TTS commands on the floor for a webview if one is already in progress? While TTS is in progress if another request is made then it all starts over. This use case is analogous to Safari. I am not sure if TTS itself does these kind of checks or perhaps at any given point only one request exists in the queue.
https://codereview.chromium.org/342143004/diff/140001/content/renderer/render... File content/renderer/render_view_browsertest_mac.mm (right): https://codereview.chromium.org/342143004/diff/140001/content/renderer/render... content/renderer/render_view_browsertest_mac.mm:143: TEST_F(RenderViewTest, OnTtsForEmptySelection) { On 2014/06/29 11:38:55, jeremy wrote: > I'm sorry but I don't understand this test. Can you please explain what part of > this CL the test is for? The patch uses contentAsText to receive web page content as text. Since there is no efficient way of testing TTS, it would be ideal to check if text returned from a SelectAll() call would be same as getContentAsText(). Moreover using WebLocalFrame::getSelectionAsText() would be analogous to a SelectAll() call. Also the need to scroll to a certain location on the page confirms regardless what the focussed frame of an webview is getContentAsText would work independent of a gfx::Rect. Hence the test. Let me know if this make sense. Thanks
On 2014/06/30 16:18:04, eka508 wrote: > https://codereview.chromium.org/342143004/diff/140001/content/renderer/render... > File content/renderer/render_view_browsertest_mac.mm (right): > > https://codereview.chromium.org/342143004/diff/140001/content/renderer/render... > content/renderer/render_view_browsertest_mac.mm:143: TEST_F(RenderViewTest, > OnTtsForEmptySelection) { > On 2014/06/29 11:38:55, jeremy wrote: > > I'm sorry but I don't understand this test. Can you please explain what part > of > > this CL the test is for? > > The patch uses contentAsText to receive web page content as text. Since there is > no efficient way of testing TTS, it would be ideal to check if text returned > from a SelectAll() call would be same as getContentAsText(). Moreover using > WebLocalFrame::getSelectionAsText() would be analogous to a SelectAll() call. > > Also the need to scroll to a certain location on the page confirms regardless > what the focussed frame of an webview is getContentAsText would work independent > of a gfx::Rect. > > Hence the test. > > Let me know if this make sense. > > Thanks I'm not the right person to be reviewing mac RWHV code so removing myself from reviewer list.
https://codereview.chromium.org/342143004/diff/140001/content/renderer/render... File content/renderer/render_view_impl.cc (right): https://codereview.chromium.org/342143004/diff/140001/content/renderer/render... content/renderer/render_view_impl.cc:1024: std::numeric_limits<size_t>::max()).utf8(); this could be gigantic and there's a fairly small limit on the size of IPCs that the browser will accept before it kills the render process. did you test this on a page with lots of text?
https://codereview.chromium.org/342143004/diff/140001/content/renderer/render... File content/renderer/render_view_impl.cc (right): https://codereview.chromium.org/342143004/diff/140001/content/renderer/render... content/renderer/render_view_impl.cc:1024: std::numeric_limits<size_t>::max()).utf8(); On 2014/07/01 01:33:29, jamesr wrote: > this could be gigantic and there's a fairly small limit on the size of IPCs that > the browser will accept before it kills the render process. did you test this on > a page with lots of text? Apparently I haven't tested with lots of text. I wouldn't mind taking an alternate route though. Doesn't WebContents->SelectAll() do something similar?
Some thoughts about lots of text: - Does Cocoa API support incrementally appending more text to the text that's currently spoken? If so, we could send the text in manageable chunks via multiple IPCs and keep adding to it. - If the API doesn't support the above, we could still split up the text into multiple chunks on the renderer-side and send via multiple IPCs. The browser can then wait for all of them to be received and reconstruct the string before invoking the TTS API. I'm also OK with just truncating long text sent over in this initial CL and putting a TODO/filing a bug to address the above in a follow-up CL.
On 2014/07/04 12:54:21, asvitkine (OOO back July 14th) wrote: > Some thoughts about lots of text: > > - Does Cocoa API support incrementally appending more text to the text that's > currently spoken? If so, we could send the text in manageable chunks via > multiple IPCs and keep adding to it. > - If the API doesn't support the above, we could still split up the text into > multiple chunks on the renderer-side and send via multiple IPCs. The browser can > then wait for all of them to be received and reconstruct the string before > invoking the TTS API. > > I'm also OK with just truncating long text sent over in this initial CL and > putting a TODO/filing a bug to address the above in a follow-up CL. Is this the max an IPC channel can handle ? static const size_t kMaximumMessageSize = 128 * 1024 * 1024 from ipc_channel.h Just some thoughts would it be better to let IPC handle all of this work instead doing one-off for a patch. For this CL I could chunk up data and send back via multiple IPC and create a bug to improve how IPC channel would be able to handle huge amounts of data. Let me know if that sounds right. Thanks
On 2014/07/06 16:39:43, eka508 wrote: > On 2014/07/04 12:54:21, asvitkine (OOO back July 14th) wrote: > > Some thoughts about lots of text: > > > > - Does Cocoa API support incrementally appending more text to the text that's > > currently spoken? If so, we could send the text in manageable chunks via > > multiple IPCs and keep adding to it. > > - If the API doesn't support the above, we could still split up the text into > > multiple chunks on the renderer-side and send via multiple IPCs. The browser > can > > then wait for all of them to be received and reconstruct the string before > > invoking the TTS API. > > > > I'm also OK with just truncating long text sent over in this initial CL and > > putting a TODO/filing a bug to address the above in a follow-up CL. > > Is this the max an IPC channel can handle ? > > static const size_t kMaximumMessageSize = 128 * 1024 * 1024 from ipc_channel.h > > Just some thoughts would it be better to let IPC handle all of this work instead > doing one-off for a patch. For this CL I could chunk up data and send back via > multiple IPC and create a bug to improve how IPC channel would be able to handle > huge amounts of data. > > Let me know if that sounds right. > > Thanks I created a new defect on TTS session can be improved to handle larger text https://code.google.com/p/chromium/issues/detail?id=393444. I will make changes suggested above by other reviewers and let the long texts truncated as of now. Thanks
Created a defect to improve how IPC could handle larger message sizes http://crbug.com/393444 PTAL Thanks
lgtm with a few improvements suggested https://codereview.chromium.org/342143004/diff/180001/content/common/view_mes... File content/common/view_messages.h (right): https://codereview.chromium.org/342143004/diff/180001/content/common/view_mes... content/common/view_messages.h:887: // Sends an Async IPC request to webview for complete content of a nit: the fact that this is an async IPC and that it is being sent to a view are all reflected in the IPC declaration: IPC_MESSAGE_ROUTE0 means it's an async IPC (sync ones are _SYNC) and ViewMsg_ means it is targetted at a view. can you shorten the comment up to just have the stuff that's particular to this? Maybe "Request the complete rendered text of the web page" or something along those lines? https://codereview.chromium.org/342143004/diff/180001/content/renderer/render... File content/renderer/render_view_browsertest_mac.mm (right): https://codereview.chromium.org/342143004/diff/180001/content/renderer/render... content/renderer/render_view_browsertest_mac.mm:171: ExecuteJavaScript("window.scrollTo(0, 9030)"); hmm, why do we need to scroll? i don't think that should have any bearing on the operation of this test https://codereview.chromium.org/342143004/diff/180001/content/renderer/render... File content/renderer/render_view_impl.cc (right): https://codereview.chromium.org/342143004/diff/180001/content/renderer/render... content/renderer/render_view_impl.cc:1026: static const size_t kMaximumMessageSize = 128 * 1024 * 1024; we won't actually be able to fit a string this big into an IPC message since we'll need a few bytes for headers and lenth and the utf8 encoding of the string may use >1 byte per character. Probably safer to pick something more reasonable here (like a few meg) that's well shy of the IPC max
PTAL https://codereview.chromium.org/342143004/diff/180001/content/renderer/render... File content/renderer/render_view_impl.cc (right): https://codereview.chromium.org/342143004/diff/180001/content/renderer/render... content/renderer/render_view_impl.cc:1026: static const size_t kMaximumMessageSize = 128 * 1024 * 1024; On 2014/07/18 06:05:04, jamesr wrote: > we won't actually be able to fit a string this big into an IPC message since > we'll need a few bytes for headers and lenth and the utf8 encoding of the string > may use >1 byte per character. Probably safer to pick something more reasonable > here (like a few meg) that's well shy of the IPC max I actually picked this number from ipc_channel.h. https://code.google.com/p/chromium/codesearch#chromium/src/ipc/ipc_channel.h&...
PTAL Thanks https://codereview.chromium.org/342143004/diff/180001/content/common/view_mes... File content/common/view_messages.h (right): https://codereview.chromium.org/342143004/diff/180001/content/common/view_mes... content/common/view_messages.h:887: // Sends an Async IPC request to webview for complete content of a On 2014/07/18 06:05:04, jamesr wrote: > nit: the fact that this is an async IPC and that it is being sent to a view are > all reflected in the IPC declaration: > > IPC_MESSAGE_ROUTE0 means it's an async IPC (sync ones are _SYNC) and ViewMsg_ > means it is targetted at a view. > > can you shorten the comment up to just have the stuff that's particular to this? > Maybe "Request the complete rendered text of the web page" or something along > those lines? Done. https://codereview.chromium.org/342143004/diff/180001/content/renderer/render... File content/renderer/render_view_browsertest_mac.mm (right): https://codereview.chromium.org/342143004/diff/180001/content/renderer/render... content/renderer/render_view_browsertest_mac.mm:171: ExecuteJavaScript("window.scrollTo(0, 9030)"); On 2014/07/18 06:05:04, jamesr wrote: > hmm, why do we need to scroll? i don't think that should have any bearing on the > operation of this test Done.
The CQ bit was checked by a.sarkar.arun@gmail.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/a.sarkar.arun@gmail.com/342143004/200001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: chromium_presubmit on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
On 2014/07/19 06:27:35, eka508 wrote: > PTAL > > https://codereview.chromium.org/342143004/diff/180001/content/renderer/render... > File content/renderer/render_view_impl.cc (right): > > https://codereview.chromium.org/342143004/diff/180001/content/renderer/render... > content/renderer/render_view_impl.cc:1026: static const size_t > kMaximumMessageSize = 128 * 1024 * 1024; > On 2014/07/18 06:05:04, jamesr wrote: > > we won't actually be able to fit a string this big into an IPC message since > > we'll need a few bytes for headers and lenth and the utf8 encoding of the > string > > may use >1 byte per character. Probably safer to pick something more > reasonable > > here (like a few meg) that's well shy of the IPC max > > I actually picked this number from ipc_channel.h. > > https://code.google.com/p/chromium/codesearch#chromium/src/ipc/ipc_channel.h&... I know, and I explained why that number won't actually work. If you disagree with a reviewer's comments you should address them before landing, not ignore them and try to land anyway.
On 2014/07/21 19:28:04, jamesr wrote: > On 2014/07/19 06:27:35, eka508 wrote: > > PTAL > > > > > https://codereview.chromium.org/342143004/diff/180001/content/renderer/render... > > File content/renderer/render_view_impl.cc (right): > > > > > https://codereview.chromium.org/342143004/diff/180001/content/renderer/render... > > content/renderer/render_view_impl.cc:1026: static const size_t > > kMaximumMessageSize = 128 * 1024 * 1024; > > On 2014/07/18 06:05:04, jamesr wrote: > > > we won't actually be able to fit a string this big into an IPC message since > > > we'll need a few bytes for headers and lenth and the utf8 encoding of the > > string > > > may use >1 byte per character. Probably safer to pick something more > > reasonable > > > here (like a few meg) that's well shy of the IPC max > > > > I actually picked this number from ipc_channel.h. > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/ipc/ipc_channel.h&... > > I know, and I explained why that number won't actually work. If you disagree > with a reviewer's comments you should address them before landing, not ignore > them and try to land anyway. My apologies. My original intention however was just to run tests on bots and see if they passed or all not, re-reading the docs I figured out I could do that only if I had permissions to run git cl try, so i clicked the 'commit' checkbox to see what happens. Lesson learnt. As far as the patch goes , I may be completely misunderstading what kMaximumMessageSize is and how is it used? In such scenarios what sizes are safe for sending strings over IPC. Thanks
On 2014/07/21 19:28:04, jamesr wrote: > On 2014/07/19 06:27:35, eka508 wrote: > > PTAL > > > > > https://codereview.chromium.org/342143004/diff/180001/content/renderer/render... > > File content/renderer/render_view_impl.cc (right): > > > > > https://codereview.chromium.org/342143004/diff/180001/content/renderer/render... > > content/renderer/render_view_impl.cc:1026: static const size_t > > kMaximumMessageSize = 128 * 1024 * 1024; > > On 2014/07/18 06:05:04, jamesr wrote: > > > we won't actually be able to fit a string this big into an IPC message since > > > we'll need a few bytes for headers and lenth and the utf8 encoding of the > > string > > > may use >1 byte per character. Probably safer to pick something more > > reasonable > > > here (like a few meg) that's well shy of the IPC max > > > > I actually picked this number from ipc_channel.h. > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/ipc/ipc_channel.h&... > > I know, and I explained why that number won't actually work. If you disagree > with a reviewer's comments you should address them before landing, not ignore > them and try to land anyway. I overlooked the comment over kMaximumMessageSize. I would re-upload a patch with a message size that is more reasonable. Thanks and sorry for the trouble.
Changed the message size to something more reasonable ~8.5mb PTAL Thanks
@thakis, @jamesr could you PTAL again. Thanks
content/renderer lgtm https://codereview.chromium.org/342143004/diff/220001/content/renderer/render... File content/renderer/render_view_impl.cc (right): https://codereview.chromium.org/342143004/diff/220001/content/renderer/render... content/renderer/render_view_impl.cc:1026: static const size_t kMaximumMessageSize = 64 * 128 * 1024; 64 * 128 * 1024 is hard to parse. can you just say 8 * 1024 * 1024 ?
Updated chunk size. @thakis PTAL Thanks
I think I would need a LGTM from content/browser/render_host. Thanks https://codereview.chromium.org/342143004/diff/220001/content/renderer/render... File content/renderer/render_view_impl.cc (right): https://codereview.chromium.org/342143004/diff/220001/content/renderer/render... content/renderer/render_view_impl.cc:1026: static const size_t kMaximumMessageSize = 64 * 128 * 1024; On 2014/07/28 22:19:26, jamesr wrote: > 64 * 128 * 1024 is hard to parse. can you just say 8 * 1024 * 1024 ? Done.
On 2014/07/30 15:56:58, eka508 wrote: > I think I would need a LGTM from content/browser/render_host. > > Thanks > > https://codereview.chromium.org/342143004/diff/220001/content/renderer/render... > File content/renderer/render_view_impl.cc (right): > > https://codereview.chromium.org/342143004/diff/220001/content/renderer/render... > content/renderer/render_view_impl.cc:1026: static const size_t > kMaximumMessageSize = 64 * 128 * 1024; > On 2014/07/28 22:19:26, jamesr wrote: > > 64 * 128 * 1024 is hard to parse. can you just say 8 * 1024 * 1024 ? > > Done. PTAL
On 2014/08/05 00:07:12, eka508 wrote: > On 2014/07/30 15:56:58, eka508 wrote: > > I think I would need a LGTM from content/browser/render_host. > > > > Thanks > > > > > https://codereview.chromium.org/342143004/diff/220001/content/renderer/render... > > File content/renderer/render_view_impl.cc (right): > > > > > https://codereview.chromium.org/342143004/diff/220001/content/renderer/render... > > content/renderer/render_view_impl.cc:1026: static const size_t > > kMaximumMessageSize = 64 * 128 * 1024; > > On 2014/07/28 22:19:26, jamesr wrote: > > > 64 * 128 * 1024 is hard to parse. can you just say 8 * 1024 * 1024 ? > > > > Done. > > PTAL Include couple of more reviewers for IPC message changes. PTAL Thanks
I'm wondering why this patch is Mac-only. Why isn't this flaw present on other platforms? The patch description should explain that IMO. https://codereview.chromium.org/342143004/diff/240001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_mac.mm (right): https://codereview.chromium.org/342143004/diff/240001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_mac.mm:2121: if (text.empty()) Why does the empty case need special handling? https://codereview.chromium.org/342143004/diff/240001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_mac.mm:2968: - (void)speakText:(NSString *) text { NSString* https://codereview.chromium.org/342143004/diff/240001/content/renderer/render... File content/renderer/render_view_browsertest_mac.mm (right): https://codereview.chromium.org/342143004/diff/240001/content/renderer/render... content/renderer/render_view_browsertest_mac.mm:143: TEST_F(RenderViewTest, OnTtsForEmptySelection) { This test feels out of place in Chrome. contentAsText() is not a chrome or content API; it's a Blink API. If we want to make sure it's doing the right thing, the right place to test it is in the Blink layer, where the API is defined. https://codereview.chromium.org/342143004/diff/240001/content/renderer/render... content/renderer/render_view_browsertest_mac.mm:144: #define HTML(s) #s Is this really a common idiom? I think it's better to just use quotes and a shorter test case.
Can you please find another IPC security reviewer? Thanks, I'm too busy at the moment.
On 2014/08/06 02:09:29, dcheng (OOO) wrote: > I'm wondering why this patch is Mac-only. Why isn't this flaw present on other > platforms? The patch description should explain that IMO. > > https://codereview.chromium.org/342143004/diff/240001/content/browser/rendere... > File content/browser/renderer_host/render_widget_host_view_mac.mm (right): > I think this is Mac only defect as explained in crbug.com/248426. > https://codereview.chromium.org/342143004/diff/240001/content/browser/rendere... > content/browser/renderer_host/render_widget_host_view_mac.mm:2121: if > (text.empty()) > Why does the empty case need special handling? > Just an early return. > https://codereview.chromium.org/342143004/diff/240001/content/browser/rendere... > content/browser/renderer_host/render_widget_host_view_mac.mm:2968: - > (void)speakText:(NSString *) text { > NSString* > Done > https://codereview.chromium.org/342143004/diff/240001/content/renderer/render... > File content/renderer/render_view_browsertest_mac.mm (right): > > https://codereview.chromium.org/342143004/diff/240001/content/renderer/render... > content/renderer/render_view_browsertest_mac.mm:143: TEST_F(RenderViewTest, > OnTtsForEmptySelection) { > This test feels out of place in Chrome. contentAsText() is not a chrome or > content API; it's a Blink API. If we want to make sure it's doing the right > thing, the right place to test it is in the Blink layer, where the API is > defined. > contentAsText() is used in render_view_browsertest. Although the test doesn't use an async IPC to test the newly included signatures in view_messages.h it does however asserts text returned from selectAll is same as contentAsText hence it would be appropriate to a mac specific browser tests. > https://codereview.chromium.org/342143004/diff/240001/content/renderer/render... > content/renderer/render_view_browsertest_mac.mm:144: #define HTML(s) #s > Is this really a common idiom? I think it's better to just use quotes and a > shorter test case. Not sure but just keeping it consistent with the original file :). PTAL Thanks
On 2014/08/08 18:49:46, eka508 wrote: > On 2014/08/06 02:09:29, dcheng (OOO) wrote: > > I'm wondering why this patch is Mac-only. Why isn't this flaw present on other > > platforms? The patch description should explain that IMO. > > > > > https://codereview.chromium.org/342143004/diff/240001/content/browser/rendere... > > File content/browser/renderer_host/render_widget_host_view_mac.mm (right): > > > I think this is Mac only defect as explained in crbug.com/248426. I don't see any explanation of why this is Mac only in the bug. > > > https://codereview.chromium.org/342143004/diff/240001/content/browser/rendere... > > content/browser/renderer_host/render_widget_host_view_mac.mm:2121: if > > (text.empty()) > > Why does the empty case need special handling? > > > Just an early return. If the early return is non critical, it just leads to redundant checks. Please remove if it's not required. > > > > https://codereview.chromium.org/342143004/diff/240001/content/browser/rendere... > > content/browser/renderer_host/render_widget_host_view_mac.mm:2968: - > > (void)speakText:(NSString *) text { > > NSString* > > > > Done > > > > https://codereview.chromium.org/342143004/diff/240001/content/renderer/render... > > File content/renderer/render_view_browsertest_mac.mm (right): > > > > > https://codereview.chromium.org/342143004/diff/240001/content/renderer/render... > > content/renderer/render_view_browsertest_mac.mm:143: TEST_F(RenderViewTest, > > OnTtsForEmptySelection) { > > This test feels out of place in Chrome. contentAsText() is not a chrome or > > content API; it's a Blink API. If we want to make sure it's doing the right > > thing, the right place to test it is in the Blink layer, where the API is > > defined. > > > contentAsText() is used in render_view_browsertest. > Although the test doesn't use an async IPC to test the newly included signatures > in view_messages.h it does however asserts text returned from selectAll is same > as contentAsText hence it would be appropriate to a mac specific browser tests. > Regardless of what render_view_browser test.cc does, this is still the wrong layer for this test. It tests nothing content specific, nor does it test TTS. If you want to test this, the right place for this test is in Source/web/tests in Blink. > > > https://codereview.chromium.org/342143004/diff/240001/content/renderer/render... > > content/renderer/render_view_browsertest_mac.mm:144: #define HTML(s) #s > > Is this really a common idiom? I think it's better to just use quotes and a > > shorter test case. > Not sure but just keeping it consistent with the original file :). IMO, just use a simpler test case and avoid the need for a #define at all. But I don't feel strongly about this. > > PTAL > Thanks
On 2014/08/08 20:16:36, dcheng (OOO) wrote: > On 2014/08/08 18:49:46, eka508 wrote: > > On 2014/08/06 02:09:29, dcheng (OOO) wrote: > > > I'm wondering why this patch is Mac-only. Why isn't this flaw present on > other > > > platforms? The patch description should explain that IMO. > > > > > > > > > https://codereview.chromium.org/342143004/diff/240001/content/browser/rendere... > > > File content/browser/renderer_host/render_widget_host_view_mac.mm (right): > > > > > I think this is Mac only defect as explained in crbug.com/248426. > > I don't see any explanation of why this is Mac only in the bug. > > > > > > > https://codereview.chromium.org/342143004/diff/240001/content/browser/rendere... > > > content/browser/renderer_host/render_widget_host_view_mac.mm:2121: if > > > (text.empty()) > > > Why does the empty case need special handling? > > > > > Just an early return. > > If the early return is non critical, it just leads to redundant checks. Please > remove if it's not required. > > > > > > > > > https://codereview.chromium.org/342143004/diff/240001/content/browser/rendere... > > > content/browser/renderer_host/render_widget_host_view_mac.mm:2968: - > > > (void)speakText:(NSString *) text { > > > NSString* > > > > > > > Done > > > > > > > > https://codereview.chromium.org/342143004/diff/240001/content/renderer/render... > > > File content/renderer/render_view_browsertest_mac.mm (right): > > > > > > > > > https://codereview.chromium.org/342143004/diff/240001/content/renderer/render... > > > content/renderer/render_view_browsertest_mac.mm:143: TEST_F(RenderViewTest, > > > OnTtsForEmptySelection) { > > > This test feels out of place in Chrome. contentAsText() is not a chrome or > > > content API; it's a Blink API. If we want to make sure it's doing the right > > > thing, the right place to test it is in the Blink layer, where the API is > > > defined. > > > > > contentAsText() is used in render_view_browsertest. > > Although the test doesn't use an async IPC to test the newly included > signatures > > in view_messages.h it does however asserts text returned from selectAll is > same > > as contentAsText hence it would be appropriate to a mac specific browser > tests. > > > > Regardless of what render_view_browser test.cc does, this is still the wrong > layer for this test. It tests nothing content specific, nor does it test TTS. If > you want to test this, the right place for this test is in Source/web/tests in > Blink. > As of now I don't have Blink checked out , once its done I will upload a patch with changes to WebViewTest.cpp and link it to this CL. Thanks > > > > > > https://codereview.chromium.org/342143004/diff/240001/content/renderer/render... > > > content/renderer/render_view_browsertest_mac.mm:144: #define HTML(s) #s > > > Is this really a common idiom? I think it's better to just use quotes and a > > > shorter test case. > > Not sure but just keeping it consistent with the original file :). > > IMO, just use a simpler test case and avoid the need for a #define at all. But > I don't feel strongly about this. > > > > > PTAL > > Thanks
On 2014/08/11 16:28:33, sarka wrote: > On 2014/08/08 20:16:36, dcheng (OOO) wrote: > > On 2014/08/08 18:49:46, eka508 wrote: > > > On 2014/08/06 02:09:29, dcheng (OOO) wrote: > > > > I'm wondering why this patch is Mac-only. Why isn't this flaw present on > > other > > > > platforms? The patch description should explain that IMO. > > > > > > > > > > > > > > https://codereview.chromium.org/342143004/diff/240001/content/browser/rendere... > > > > File content/browser/renderer_host/render_widget_host_view_mac.mm (right): > > > > > > > I think this is Mac only defect as explained in crbug.com/248426. > > > > I don't see any explanation of why this is Mac only in the bug. > > > > > > From my understanding the text to speech doesn't even exist in other platforms. For instance this particular feature on Mac can be accessed via context menu, not sure if there are any context menus in other platforms. > > > > > > https://codereview.chromium.org/342143004/diff/240001/content/browser/rendere... > > > > content/browser/renderer_host/render_widget_host_view_mac.mm:2121: if > > > > (text.empty()) > > > > Why does the empty case need special handling? > > > > > > > Just an early return. > > > > If the early return is non critical, it just leads to redundant checks. Please > > remove if it's not required. > > > > > > > > > Done > > > > > > https://codereview.chromium.org/342143004/diff/240001/content/browser/rendere... > > > > content/browser/renderer_host/render_widget_host_view_mac.mm:2968: - > > > > (void)speakText:(NSString *) text { > > > > NSString* > > > > > > > > > > Done > > > > > > > > > > > > > https://codereview.chromium.org/342143004/diff/240001/content/renderer/render... > > > > File content/renderer/render_view_browsertest_mac.mm (right): > > > > > > > > > > > > > > https://codereview.chromium.org/342143004/diff/240001/content/renderer/render... > > > > content/renderer/render_view_browsertest_mac.mm:143: > TEST_F(RenderViewTest, > > > > OnTtsForEmptySelection) { > > > > This test feels out of place in Chrome. contentAsText() is not a chrome or > > > > content API; it's a Blink API. If we want to make sure it's doing the > right > > > > thing, the right place to test it is in the Blink layer, where the API is > > > > defined. > > > > > > > contentAsText() is used in render_view_browsertest. > > > Although the test doesn't use an async IPC to test the newly included > > signatures > > > in view_messages.h it does however asserts text returned from selectAll is > > same > > > as contentAsText hence it would be appropriate to a mac specific browser > > tests. > > > > > > > Regardless of what render_view_browser test.cc does, this is still the wrong > > layer for this test. It tests nothing content specific, nor does it test TTS. > If > > you want to test this, the right place for this test is in Source/web/tests in > > Blink. > > > > As of now I don't have Blink checked out , once its done I will upload a patch > with changes to WebViewTest.cpp and link it to this CL. > Thanks > > > > > > > > > > https://codereview.chromium.org/342143004/diff/240001/content/renderer/render... > > > > content/renderer/render_view_browsertest_mac.mm:144: #define HTML(s) #s > > > > Is this really a common idiom? I think it's better to just use quotes and > a > > > > shorter test case. > > > Not sure but just keeping it consistent with the original file :). > > > > IMO, just use a simpler test case and avoid the need for a #define at all. > But > > I don't feel strongly about this. > > > > > Done Here is a link to the CL https://codereview.chromium.org/464613003/ > > > PTAL > > > Thanks PTAL Thanks
https://codereview.chromium.org/342143004/diff/300001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_mac.mm (right): https://codereview.chromium.org/342143004/diff/300001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_mac.mm:3049: - (void)speakText:(NSString*) text { Nit: not really sure why this is currently it's own method. You could change this to a normal C++ method and have it take a const std::string& so you don't have to repeat the call to SysUTF8ToNSString() in both OnGetRenderedTextComplete() and SpeakSelection(). Otherwise, just inline this into the two places it's called: [NSApp speakText:...] is shorter than [cocoa_view_ speakText:...] anyway.
https://codereview.chromium.org/342143004/diff/300001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_mac.mm (right): https://codereview.chromium.org/342143004/diff/300001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_mac.mm:3049: - (void)speakText:(NSString*) text { On 2014/08/12 20:55:27, dcheng (OOO) wrote: > Nit: not really sure why this is currently it's own method. You could change > this to a normal C++ method and have it take a const std::string& so you don't > have to repeat the call to SysUTF8ToNSString() in both > OnGetRenderedTextComplete() and SpeakSelection(). > > Otherwise, just inline this into the two places it's called: [NSApp > speakText:...] is shorter than [cocoa_view_ speakText:...] anyway. I think [NSApp speakString] is decided at runtime since its an undocumented method and one of the reviewers suggested from an earlier patch that I make a private method that can be used launch a TTS session hence [cocoa_view_ speakText:]. Let me know if that makes sense, else I would make suggested changes.
On 2014/08/12 21:03:17, sarka wrote: > https://codereview.chromium.org/342143004/diff/300001/content/browser/rendere... > File content/browser/renderer_host/render_widget_host_view_mac.mm (right): > > https://codereview.chromium.org/342143004/diff/300001/content/browser/rendere... > content/browser/renderer_host/render_widget_host_view_mac.mm:3049: - > (void)speakText:(NSString*) text { > On 2014/08/12 20:55:27, dcheng (OOO) wrote: > > Nit: not really sure why this is currently it's own method. You could change > > this to a normal C++ method and have it take a const std::string& so you don't > > have to repeat the call to SysUTF8ToNSString() in both > > OnGetRenderedTextComplete() and SpeakSelection(). > > > > Otherwise, just inline this into the two places it's called: [NSApp > > speakText:...] is shorter than [cocoa_view_ speakText:...] anyway. > Done. Made a C++ method to dispatch sessions. PTAL. Thanks > I think [NSApp speakString] is decided at runtime since its an undocumented > method and one of the reviewers suggested from an earlier patch that I make a > private method that can be used launch a TTS session hence [cocoa_view_ > speakText:]. Let me know if that makes sense, else I would make suggested > changes.
lgtm with one nit https://codereview.chromium.org/342143004/diff/320001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_mac.mm (right): https://codereview.chromium.org/342143004/diff/320001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_mac.mm:885: void RenderWidgetHostViewMac::SpeakText(const std::string &text) { Nit: fix alignment of & (& should be by type, not by the identifier)
On 2014/08/14 17:38:48, dcheng (OOO) wrote: > lgtm with one nit > > https://codereview.chromium.org/342143004/diff/320001/content/browser/rendere... > File content/browser/renderer_host/render_widget_host_view_mac.mm (right): > > https://codereview.chromium.org/342143004/diff/320001/content/browser/rendere... > content/browser/renderer_host/render_widget_host_view_mac.mm:885: void > RenderWidgetHostViewMac::SpeakText(const std::string &text) { > Nit: fix alignment of & (& should be by type, not by the identifier) Done. PTAL. Thanks
On 2014/08/14 at 18:07:22, a.sarkar.arun wrote: > On 2014/08/14 17:38:48, dcheng (OOO) wrote: > > lgtm with one nit > > > > https://codereview.chromium.org/342143004/diff/320001/content/browser/rendere... > > File content/browser/renderer_host/render_widget_host_view_mac.mm (right): > > > > https://codereview.chromium.org/342143004/diff/320001/content/browser/rendere... > > content/browser/renderer_host/render_widget_host_view_mac.mm:885: void > > RenderWidgetHostViewMac::SpeakText(const std::string &text) { > > Nit: fix alignment of & (& should be by type, not by the identifier) > > Done. > > PTAL. > Thanks still lgtm
Still LGTM from me too with one nit. I think you have all the owner approvals now, so feel free to click the "commit queue" checkbox once you address my last comment. Thanks! https://codereview.chromium.org/342143004/diff/340001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_mac.h (right): https://codereview.chromium.org/342143004/diff/340001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_mac.h:594: void SpeakText(const std::string& text); Nit: Add a short comment.
On 2014/08/14 21:26:26, Alexei Svitkine wrote: > Still LGTM from me too with one nit. > > I think you have all the owner approvals now, so feel free to click the "commit > queue" checkbox once you address my last comment. Thanks! > > https://codereview.chromium.org/342143004/diff/340001/content/browser/rendere... > File content/browser/renderer_host/render_widget_host_view_mac.h (right): > > https://codereview.chromium.org/342143004/diff/340001/content/browser/rendere... > content/browser/renderer_host/render_widget_host_view_mac.h:594: void > SpeakText(const std::string& text); > Nit: Add a short comment. Done
The CQ bit was checked by a.sarkar.arun@gmail.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/a.sarkar.arun@gmail.com/342143004/360001
Message was sent while issue was closed.
Committed patchset #17 (360001) as 289799 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
