Issue 2164483006: [MacViews] Implemented text context menu

spqchan

Description was changed from ========== Experimenting with text context menu on MacViews lol I kinda ...

4 years, 5 months ago (2016-07-21 19:11:15 UTC) #1

spqchan

Description was changed from ========== Experimenting with text context menu on MacViews lol I kinda ...

4 years, 5 months ago (2016-07-21 23:43:11 UTC) #2

spqchan

Description was changed from ========== Experimenting with text context menu on MacViews ========== to ========== ...

4 years, 5 months ago (2016-07-21 23:47:53 UTC) #3

tapted

tapted@chromium.org changed reviewers: + tapted@chromium.org

4 years, 5 months ago (2016-07-22 03:06:40 UTC) #4

tapted

this direction looks pretty good! left some suggestions https://codereview.chromium.org/2164483006/diff/140001/chrome/browser/ui/cocoa/renderer_context_menu/render_view_context_menu_mac.h File chrome/browser/ui/cocoa/renderer_context_menu/render_view_context_menu_mac.h (right): https://codereview.chromium.org/2164483006/diff/140001/chrome/browser/ui/cocoa/renderer_context_menu/render_view_context_menu_mac.h#newcode21 chrome/browser/ui/cocoa/renderer_context_menu/render_view_context_menu_mac.h:21: TextContextMenu::Delegate ...

4 years, 5 months ago (2016-07-22 03:06:43 UTC) #5

spqchan

Description was changed from ========== Experimenting with text context menu on MacViews - Creating a ...

4 years, 4 months ago (2016-07-28 20:28:00 UTC) #6

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2164483006/340001

4 years ago (2016-12-10 01:39:53 UTC) #8

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

4 years ago (2016-12-10 02:11:34 UTC) #9

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_compile_dbg_ng/builds/321310)

4 years ago (2016-12-10 02:11:35 UTC) #10

spqchan

Description was changed from ========== Experimenting with text context menu on MacViews - Creating a ...

4 years ago (2016-12-10 02:26:18 UTC) #11

spqchan

Description was changed from ========== [MacViews] Implemented text context menu - Added Speech, Writing Direction ...

4 years ago (2016-12-10 02:26:42 UTC) #12

spqchan

Description was changed from ========== [MacViews] Implemented text context menu - Added Speech, Writing Direction ...

4 years ago (2016-12-10 02:30:21 UTC) #13

spqchan

Description was changed from ========== [MacViews] Implemented text context menu - Added Speech, Writing Direction ...

4 years ago (2016-12-10 02:46:09 UTC) #14

spqchan

PTAL Haha sorry it took so long. It was a bit tricky to go through ...

4 years ago (2016-12-12 19:32:28 UTC) #15

PTAL

Haha sorry it took so long. It was a bit tricky to go through some > 4 month
comments :)

https://codereview.chromium.org/2164483006/diff/140001/chrome/browser/ui/coco...
File
chrome/browser/ui/cocoa/renderer_context_menu/render_view_context_menu_mac.h
(right):

https://codereview.chromium.org/2164483006/diff/140001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/renderer_context_menu/render_view_context_menu_mac.h:21:
TextContextMenu::Delegate {
On 2016/07/22 03:06:42, tapted wrote:
> public

Done.

https://codereview.chromium.org/2164483006/diff/140001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/renderer_context_menu/render_view_context_menu_mac.h:75:
std::unique_ptr<TextContextMenu> text_context_menu_;
On 2016/07/22 03:06:42, tapted wrote:
> could this just be a value member? -- no need for unique_ptr

Done.

https://codereview.chromium.org/2164483006/diff/140001/content/browser/render...
File content/browser/renderer_host/render_widget_host_view_mac.mm (right):

https://codereview.chromium.org/2164483006/diff/140001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_mac.mm:142: // These are
not documented, so use only after checking -respondsToSelector:.
On 2016/07/22 03:06:42, tapted wrote:
> Can these all be deleted? (if callers remain in RWHVM they should call
> text_context_menu instead)

Done.

https://codereview.chromium.org/2164483006/diff/140001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_mac.mm:1179:
TextContextMenu::SpeakText(base::ASCIIToUTF16(selected_text_));
On 2016/07/22 03:06:42, tapted wrote:
> ASCII -> UTF8
> 
> (but also ... the selected_text_ member should really be a base::string16 so
> that it's not first converted _from_ UTF16 in
> RenderWidgetHostViewMac::SelectionChanged - but that should be a separate CL)

Done.

https://codereview.chromium.org/2164483006/diff/140001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_mac.mm:1759:
TextContextMenu::SpeakText(base::ASCIIToUTF16(text));
On 2016/07/22 03:06:42, tapted wrote:
> ASCII -> UTF8

Done.

https://codereview.chromium.org/2164483006/diff/140001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_mac.mm:3319: -
(void)startSpeaking:(id)sender {
On 2016/07/22 03:06:42, tapted wrote:
> BridgedContentView will need to implement these if we want it to respond to
the
> items in the mainMenu bar (as well as the context menu). They can be
implemented
> in terms of the textInputClient_ member on BridgedContentView (see later
> comment)
> 
> It can probably be implemented something like
> 
> ViewsTextServicesMenuMac::SpeakTextInputClient(textInputClient_) -- see later
> comments
> 
> 
> (and SpeakTextInputClient could possibly even go on TextServicesContextMenu,
> since I think it's allowed to depend on ui::TextInputClient [but not views])

I'm a bit confused with what you mean about BridgedContentView since the speak
menu on the main menu is current working with MacViews. Can you elaborate?
Thanks!

https://codereview.chromium.org/2164483006/diff/140001/ui/base/cocoa/text_con...
File ui/base/cocoa/text_context_menu.h (right):

https://codereview.chromium.org/2164483006/diff/140001/ui/base/cocoa/text_con...
ui/base/cocoa/text_context_menu.h:12: class TextContextMenu : public
ui::SimpleMenuModel::Delegate {
On 2016/07/22 03:06:42, tapted wrote:
> Maybe TextServicesContextMenu? (also namespace ui. the class should have a
> comment too)

Done.

https://codereview.chromium.org/2164483006/diff/140001/ui/base/cocoa/text_con...
ui/base/cocoa/text_context_menu.h:16: virtual void StartSpeaking() = 0;
On 2016/07/22 03:06:42, tapted wrote:
> I think we should rename this to something like
> 
> OnSpeakRequested()
> 
> alternatively, the separation might be nicer with something like:
> 
> virtual void ProvideTextForSpeaking(const base::Callback<void(std::string16)>&
> callback) = 0;
> 
> This hides details about speech operation from the delegate -- it just becomes
a
> "selected text provider"
> 
> But since |callback| will always be &SpeakText, I couldn't convince myself
that
> this is actually any better than having the delegate just call SpeakText. (but
> OnSpeakRequested() should get a comment saying the delegate is expected to
call
> SpeakText)

Done.

https://codereview.chromium.org/2164483006/diff/140001/ui/base/cocoa/text_con...
ui/base/cocoa/text_context_menu.h:19: TextContextMenu(Delegate* delegate);
On 2016/07/22 03:06:42, tapted wrote:
> explicit

Done.

https://codereview.chromium.org/2164483006/diff/140001/ui/base/cocoa/text_con...
File ui/base/cocoa/text_context_menu.mm (right):

https://codereview.chromium.org/2164483006/diff/140001/ui/base/cocoa/text_con...
ui/base/cocoa/text_context_menu.mm:11: #include
"chrome/app/chrome_command_ids.h"
On 2016/07/22 03:06:42, tapted wrote:
> we can't add a chrome dependency here, but I don't think
> IDC_CONTENT_CONTEXT_SPEECH_START/STOP_SPEAKING are used for anything else --
> even their numeric values don't appear in any .xib files, so it should be fine
> simply to use IDS_SPEECH_START_SPEAKING_MAC as both the command id and the
> string id, then delete the IDC_ versions from chrome_command_ids.h

Done.

https://codereview.chromium.org/2164483006/diff/140001/ui/base/cocoa/text_con...
ui/base/cocoa/text_context_menu.mm:15: // These are not documented, so use only
after checking -respondsToSelector:.
On 2016/07/22 03:06:42, tapted wrote:
> huh.. we should probably use the public API, SpeakCFString etc. but not in
this
> CL

Done.

https://codereview.chromium.org/2164483006/diff/140001/ui/base/cocoa/text_con...
ui/base/cocoa/text_context_menu.mm:25: void TextContextMenu::SpeakText(const
base::string16& text) {
On 2016/07/22 03:06:42, tapted wrote:
> (ensure function order matches header

Done.

https://codereview.chromium.org/2164483006/diff/140001/ui/base/cocoa/text_con...
ui/base/cocoa/text_context_menu.mm:66: default:
On 2016/07/22 03:06:42, tapted wrote:
> default not needed

Done.

https://codereview.chromium.org/2164483006/diff/140001/ui/base/cocoa/text_con...
ui/base/cocoa/text_context_menu.mm:84: default:
On 2016/07/22 03:06:42, tapted wrote:
> default case not needed

Done.

https://codereview.chromium.org/2164483006/diff/140001/ui/views/controls/text...
File ui/views/controls/textfield/textfield.cc (right):

https://codereview.chromium.org/2164483006/diff/140001/ui/views/controls/text...
ui/views/controls/textfield/textfield.cc:1919: if (!text_context_menu_.get()) {
On 2016/07/22 03:06:42, tapted wrote:
> this `if` check shouldn't be needed..

Done.

https://codereview.chromium.org/2164483006/diff/140001/ui/views/controls/text...
ui/views/controls/textfield/textfield.cc:1921:
text_context_menu_->UpdateContextMenu(context_menu_contents_.get());
On 2016/07/22 03:06:42, tapted wrote:
> but there should be a check before this to ensure that Create() didn't just
> return nullptr

Removed text_context_menu_

https://codereview.chromium.org/2164483006/diff/140001/ui/views/controls/text...
File ui/views/controls/textfield/views_text_context_menu.h (right):

https://codereview.chromium.org/2164483006/diff/140001/ui/views/controls/text...
ui/views/controls/textfield/views_text_context_menu.h:22: virtual void
UpdateContextMenu(ui::SimpleMenuModel* menu) = 0;
On 2016/07/22 03:06:42, tapted wrote:
> Can |menu| just be passed in via |Create|?

Done.

https://codereview.chromium.org/2164483006/diff/140001/ui/views/controls/text...
File ui/views/controls/textfield/views_text_context_menu_mac.cc (right):

https://codereview.chromium.org/2164483006/diff/140001/ui/views/controls/text...
ui/views/controls/textfield/views_text_context_menu_mac.cc:11:
ViewsTextContextMenu* ViewsTextContextMenu::Create(Textfield* textfield) {
On 2016/07/22 03:06:42, tapted wrote:
> // static

Done.

https://codereview.chromium.org/2164483006/diff/140001/ui/views/controls/text...
ui/views/controls/textfield/views_text_context_menu_mac.cc:27: base::string16
text = textfield_->GetSelectedText();
On 2016/07/22 03:06:42, tapted wrote:
> instead of GetSelectedText, this could use the TextInputClient API and 
> 
> GetSelectionRange(&foo)
> GetTextFromRange(foo, &text);
> 
> this has a benefit of not trying to read out password fields, and it reduces
the
> coupling to `views`. I'm unsure if this is better though - it might be wrong
to
> couple it with IME. And it's not always "input" that we can read out.
> 
> The coupling to views could also be done by having ViewsTextContextMenuMac
> declare its own, custom delegate interface, and have Textfield implement it, 
> rather than just leaning on Textfield's full interface.
> 
> Right now, I'm leaning towards using TextInputClient. We should also support
> speaking when there is no selected text. This is also straightforward-- if the
> range reported by GetSelectionRange is empty, just get a range from
> GetTextRange() to read out the whole textfield

Sounds good, I switched to TextInputClient and moved the files

https://codereview.chromium.org/2164483006/diff/140001/ui/views/controls/text...
File ui/views/controls/textfield/views_text_context_menu_mac.h (right):

https://codereview.chromium.org/2164483006/diff/140001/ui/views/controls/text...
ui/views/controls/textfield/views_text_context_menu_mac.h:1: // Copyright (c)
2012 The Chromium Authors. All rights reserved.
On 2016/07/22 03:06:43, tapted wrote:
> 2016

Done.

https://codereview.chromium.org/2164483006/diff/140001/ui/views/controls/text...
ui/views/controls/textfield/views_text_context_menu_mac.h:14: class VIEWS_EXPORT
ViewsTextContextMenuMac : public ViewsTextContextMenu {
On 2016/07/22 03:06:42, tapted wrote:
> I'd probably call this ViewsTextServicesMenuMac and put it in ui/views/cocoa.
We
> should be able to remove the `Textfield` dependency (see other comments).

Done.

https://codereview.chromium.org/2164483006/diff/140001/ui/views/controls/text...
ui/views/controls/textfield/views_text_context_menu_mac.h:16:
ViewsTextContextMenuMac(Textfield* textfield);
On 2016/07/22 03:06:42, tapted wrote:
> explicit

Done.

https://codereview.chromium.org/2164483006/diff/140001/ui/views/controls/text...
ui/views/controls/textfield/views_text_context_menu_mac.h:25:
std::unique_ptr<TextContextMenu> menu_;
On 2016/07/22 03:06:43, tapted wrote:
> value member should work

Done.

https://codereview.chromium.org/2164483006/diff/140001/ui/views/controls/text...
ui/views/controls/textfield/views_text_context_menu_mac.h:26: };
On 2016/07/22 03:06:43, tapted wrote:
> nit: DISALLOW_COPY..etc

Done.

spqchan

The CQ bit was checked by spqchan@chromium.org to run a CQ dry run

4 years ago (2016-12-12 19:35:09 UTC) #16

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2164483006/420001

4 years ago (2016-12-12 19:36:08 UTC) #17

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

4 years ago (2016-12-12 20:14:57 UTC) #18

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_rel_ng/builds/332970)

4 years ago (2016-12-12 20:14:58 UTC) #19

spqchan

Ah snap, forgot to add a views_text_services_context_menu.cc file. Going to upload a patch with the ...

4 years ago (2016-12-12 22:12:32 UTC) #20

tapted

haven't dived in yet, but just answering question https://codereview.chromium.org/2164483006/diff/140001/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/2164483006/diff/140001/content/browser/renderer_host/render_widget_host_view_mac.mm#newcode3319 content/browser/renderer_host/render_widget_host_view_mac.mm:3319: - ...

4 years ago (2016-12-12 22:32:44 UTC) #21

tapted

Make sure there's a BUG= Needs a few tests too -- the writing direction checked/not-checked ...

4 years ago (2016-12-13 05:11:24 UTC) #22

spqchan

The CQ bit was checked by spqchan@chromium.org to run a CQ dry run

4 years ago (2016-12-14 18:24:29 UTC) #23

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2164483006/460001

4 years ago (2016-12-14 18:25:18 UTC) #24

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

4 years ago (2016-12-14 18:40:09 UTC) #25

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_chromeos_ozone_rel_ng/builds/288958)

4 years ago (2016-12-14 18:40:10 UTC) #26

spqchan

The CQ bit was checked by spqchan@chromium.org to run a CQ dry run

4 years ago (2016-12-14 19:09:35 UTC) #27

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2164483006/480001

4 years ago (2016-12-14 19:10:49 UTC) #28

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

4 years ago (2016-12-14 19:34:48 UTC) #29

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linux/builds/278115)

4 years ago (2016-12-14 19:34:50 UTC) #30

spqchan

The CQ bit was checked by spqchan@chromium.org to run a CQ dry run

4 years ago (2016-12-14 22:56:28 UTC) #31

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2164483006/500001

4 years ago (2016-12-14 22:57:15 UTC) #32

spqchan

Description was changed from ========== [MacViews] Implemented text context menu - Added Speech, Writing Direction ...

4 years ago (2016-12-14 22:57:37 UTC) #33

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

4 years ago (2016-12-14 23:19:26 UTC) #34

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_chromium_compile_only_ng/builds/251906)

4 years ago (2016-12-14 23:19:28 UTC) #35

spqchan

Description was changed from ========== [MacViews] Implemented text context menu - Added Speech, Writing Direction ...

4 years ago (2016-12-15 00:23:09 UTC) #36

spqchan

The CQ bit was checked by spqchan@chromium.org to run a CQ dry run

4 years ago (2016-12-15 00:23:55 UTC) #37

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2164483006/520001

4 years ago (2016-12-15 00:24:26 UTC) #38

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

4 years ago (2016-12-15 00:30:39 UTC) #39

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_compile_dbg_ng/builds/315125) win_chromium_rel_ng on ...

4 years ago (2016-12-15 00:30:40 UTC) #40

spqchan

The CQ bit was checked by spqchan@chromium.org to run a CQ dry run

4 years ago (2016-12-15 04:31:12 UTC) #41

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2164483006/540001

4 years ago (2016-12-15 04:31:58 UTC) #42

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

4 years ago (2016-12-15 04:47:12 UTC) #43

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-generic_chromium_compile_only_ng/builds/252118)

4 years ago (2016-12-15 04:47:13 UTC) #44

spqchan

The CQ bit was checked by spqchan@chromium.org to run a CQ dry run

4 years ago (2016-12-15 17:22:16 UTC) #45

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2164483006/560001

4 years ago (2016-12-15 17:22:49 UTC) #46

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

4 years ago (2016-12-15 19:05:25 UTC) #49

spqchan

The CQ bit was checked by spqchan@chromium.org to run a CQ dry run

4 years ago (2016-12-15 23:28:32 UTC) #51

spqchan

PTAL https://codereview.chromium.org/2164483006/diff/440001/chrome/browser/ui/cocoa/renderer_context_menu/render_view_context_menu_mac.mm File chrome/browser/ui/cocoa/renderer_context_menu/render_view_context_menu_mac.mm (right): https://codereview.chromium.org/2164483006/diff/440001/chrome/browser/ui/cocoa/renderer_context_menu/render_view_context_menu_mac.mm#newcode247 chrome/browser/ui/cocoa/renderer_context_menu/render_view_context_menu_mac.mm:247: switch (command_id) { On 2016/12/13 05:11:22, tapted wrote: ...

4 years ago (2016-12-15 23:29:02 UTC) #52

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2164483006/580001

4 years ago (2016-12-15 23:29:27 UTC) #53

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

4 years ago (2016-12-16 03:42:47 UTC) #54

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_rel_ng/builds/357702)

4 years ago (2016-12-16 03:42:48 UTC) #55

tapted

(review still in progress, but I gotta run for now) https://codereview.chromium.org/2164483006/diff/440001/chrome/browser/ui/cocoa/renderer_context_menu/render_view_context_menu_mac.mm File chrome/browser/ui/cocoa/renderer_context_menu/render_view_context_menu_mac.mm (right): https://codereview.chromium.org/2164483006/diff/440001/chrome/browser/ui/cocoa/renderer_context_menu/render_view_context_menu_mac.mm#newcode296 ...

4 years ago (2016-12-16 07:01:20 UTC) #56

(review still in progress, but I gotta run for now)

https://codereview.chromium.org/2164483006/diff/440001/chrome/browser/ui/coco...
File
chrome/browser/ui/cocoa/renderer_context_menu/render_view_context_menu_mac.mm
(right):

https://codereview.chromium.org/2164483006/diff/440001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/renderer_context_menu/render_view_context_menu_mac.mm:296:
index += 1; // Place it below the separator.
On 2016/12/15 23:29:00, spqchan wrote:
> On 2016/12/13 05:11:22, tapted wrote:
> > I think the "it" here is IDS_CONTENT_CONTEXT_LOOK_UP which has been moved to
> > text_services_context_menu, so the stuff from `if
> (params_.link_url.is_empty())
> > {` should move into text_services_context_menu as well.
> > 
> > that is, InitToolkitMenu() should just be
> > 
> > void RenderViewContextMenuMac::InitToolkitMenu() {
> >   text_services_context_menu_.AppendToContextMenu(&menu_model_);
> > }
> > 
> > with the logic above implemented using delegate interfaces
> 
> I'm a bit confused. I'm pretty sure that this section is for the spellchecks
and
> adding new words into the dictionary.

The problem is that all the code between `if (params_.link_url.is_empty()) {`
and here just sets a value for |index|. That |index| was then used to insert
IDC_CONTENT_CONTEXT_LOOK_UP immediately followed by a separator. Now it just
inserts a separator at some location, but there's no link between that separator
and IDC_CONTENT_CONTEXT_LOOK_UP since that's now inserted elsewhere.

So shouldn't the InsertSeparatorAt line below (and the |index| calculation here)
happen where IDC_CONTENT_CONTEXT_LOOK_UP is inserted?

https://codereview.chromium.org/2164483006/diff/440001/ui/base/cocoa/text_ser...
File ui/base/cocoa/text_services_context_menu.mm (right):

https://codereview.chromium.org/2164483006/diff/440001/ui/base/cocoa/text_ser...
ui/base/cocoa/text_services_context_menu.mm:89: case
IDS_CONTENT_CONTEXT_LOOK_UP:
On 2016/12/15 23:29:01, spqchan wrote:
> On 2016/12/13 05:11:23, tapted wrote:
> > For the others it might be OK, but do we risk a command_id conflict using
> > IDS_CONTENT_CONTEXT_LOOK_UP here? That is, can we can guarantee that the
> > renderer context menu doesn't decide to use a command_id equal to whatever
> value
> > IDS_CONTENT_CONTEXT_LOOK_UP takes from the generated header?
> 
> That's true. AFAIK. there's no conflict now, but I'm not sure how to prevent
> this  I'm not really sure how to do it without using chrome_command_ids. 
> 
> I can include chrome_command_ids.h and use the IDC values, or copy the values
to
> text_services_menu and add a comment about that.

So one way to avoid it could be to use MenuModel rather than SimpleMenuModel.
MenuModel uses the index in the menu rather than the command_id to refer to
items. However, I think that will add extra complexity. RenderViewContextMenu is
pretty heavy on the command_id thing.

I think this does need a proper solution. I think for now we should pass the
command_id to use for "lookup" to the method that adds that item.

https://codereview.chromium.org/2164483006/diff/440001/ui/gfx/render_text.h
File ui/gfx/render_text.h (right):

https://codereview.chromium.org/2164483006/diff/440001/ui/gfx/render_text.h#n...
ui/gfx/render_text.h:732: virtual bool GetDecoratedTextForRange(const Range&
range,
On 2016/12/15 23:29:01, spqchan wrote:
> On 2016/12/13 05:11:23, tapted wrote:
> > It's confusing to have a virtual and a non-virtual method with the same
name.
> I
> > think this needs to be renamed to something like
GetDecoratedTextForRangeImpl
> 
> How about GetDecoratedTextAndBasepointForRange? If this is good, I can rename
> GetDecoratedWordAtPoint and etc to GetDecoratedWordAndBaselineAtPoint 

With GetDecoratedTextAndBasepointForRange ->
GetDecoratedTextAndBaselinePointForRange

that's good too. I don't feel strongly (so long as they're different), but a
ui/gfx/OWNER (probably msw) might have a preference.

https://codereview.chromium.org/2164483006/diff/580001/chrome/browser/ui/coco...
File
chrome/browser/ui/cocoa/renderer_context_menu/render_view_context_menu_mac.h
(right):

https://codereview.chromium.org/2164483006/diff/580001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/renderer_context_menu/render_view_context_menu_mac.h:33:
// TextServicesContextMenu:
nit: TextServicesContextMenu::Delegate:

https://codereview.chromium.org/2164483006/diff/580001/chrome/browser/ui/coco...
File
chrome/browser/ui/cocoa/renderer_context_menu/render_view_context_menu_mac.mm
(left):

https://codereview.chromium.org/2164483006/diff/580001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/renderer_context_menu/render_view_context_menu_mac.mm:334:
void RenderViewContextMenuMac::AppendPlatformEditableItems() {
There might be a regression here. RenderViewContextMenu::InitMenu() invokes this
between

  if (content_type_->SupportsGroup(
          ContextMenuContentType::ITEM_GROUP_EDITABLE)) {
    menu_model_.AddSeparator(ui::NORMAL_SEPARATOR);
    AppendLanguageSettings();
    AppendPlatformEditableItems();
  }

So, do we now add the Bidi submenu when right-clicking non-editable elements on
a web page?

https://codereview.chromium.org/2164483006/diff/580001/chrome/browser/ui/coco...
File
chrome/browser/ui/cocoa/renderer_context_menu/render_view_context_menu_mac.mm
(right):

https://codereview.chromium.org/2164483006/diff/580001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/renderer_context_menu/render_view_context_menu_mac.mm:283:
index += 1; // Place it below the separator.
per comment on prior patchset (and with the separator insert moved), |index| is
unused after this line, so the code above to calculate |index| isn't utilised.

https://codereview.chromium.org/2164483006/diff/580001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/renderer_context_menu/render_view_context_menu_mac.mm:345:
default:
nit: I think the convention is to do

  case TEXT_DIRECTION_NUM_DIRECTIONS:
}

NOTREACHED();
return false;

This is because we get a warning if someone adds to the enum. It's also clearer
that the function always returns a value -- I think MSVC is ok now with
"default:", but with the above change it would sometimes warn defensively about
a function that may not return a value.

(more below)

https://codereview.chromium.org/2164483006/diff/580001/content/browser/render...
File content/browser/renderer_host/render_widget_host_view_mac.mm (right):

https://codereview.chromium.org/2164483006/diff/580001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_mac.mm:1101: //
TODO(spqchan): Change |selected_text_|'s type to string16.
I think changing |selected_text_| can be included -- it should be an easy change
:). The TODO should then go into OnGetRenderedTextCompleted (to change the
|text| argument to string16)

https://codereview.chromium.org/2164483006/diff/580001/ui/views/controls/text...
File ui/views/controls/textfield/textfield.cc (right):

https://codereview.chromium.org/2164483006/diff/580001/ui/views/controls/text...
ui/views/controls/textfield/textfield.cc:1959: context_menu_contents_.reset();
this should have a comment

spqchan

Take your time :) I made changes according to what you put out so far ...

4 years ago (2016-12-17 00:34:21 UTC) #57

Take your time :)

I made changes according to what you put out so far

https://codereview.chromium.org/2164483006/diff/580001/chrome/browser/ui/coco...
File
chrome/browser/ui/cocoa/renderer_context_menu/render_view_context_menu_mac.h
(right):

https://codereview.chromium.org/2164483006/diff/580001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/renderer_context_menu/render_view_context_menu_mac.h:33:
// TextServicesContextMenu:
On 2016/12/16 07:01:19, tapted wrote:
> nit: TextServicesContextMenu::Delegate:

Done.

https://codereview.chromium.org/2164483006/diff/580001/chrome/browser/ui/coco...
File
chrome/browser/ui/cocoa/renderer_context_menu/render_view_context_menu_mac.mm
(left):

https://codereview.chromium.org/2164483006/diff/580001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/renderer_context_menu/render_view_context_menu_mac.mm:334:
void RenderViewContextMenuMac::AppendPlatformEditableItems() {
On 2016/12/16 07:01:19, tapted wrote:
> There might be a regression here. RenderViewContextMenu::InitMenu() invokes
this
> between
> 
>   if (content_type_->SupportsGroup(
>           ContextMenuContentType::ITEM_GROUP_EDITABLE)) {
>     menu_model_.AddSeparator(ui::NORMAL_SEPARATOR);
>     AppendLanguageSettings();
>     AppendPlatformEditableItems();
>   }
> 
> So, do we now add the Bidi submenu when right-clicking non-editable elements
on
> a web page?

Augh, can't believe I missed this. Thanks for catching it!

https://codereview.chromium.org/2164483006/diff/580001/chrome/browser/ui/coco...
File
chrome/browser/ui/cocoa/renderer_context_menu/render_view_context_menu_mac.mm
(right):

https://codereview.chromium.org/2164483006/diff/580001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/renderer_context_menu/render_view_context_menu_mac.mm:283:
index += 1; // Place it below the separator.
On 2016/12/16 07:01:19, tapted wrote:
> per comment on prior patchset (and with the separator insert moved), |index|
is
> unused after this line, so the code above to calculate |index| isn't utilised.

Doh, I see what you mean. Fixed it, thanks :)

https://codereview.chromium.org/2164483006/diff/580001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/renderer_context_menu/render_view_context_menu_mac.mm:345:
default:
On 2016/12/16 07:01:19, tapted wrote:
> nit: I think the convention is to do
> 
>   case TEXT_DIRECTION_NUM_DIRECTIONS:
> }
> 
> NOTREACHED();
> return false;
> 
> 
> This is because we get a warning if someone adds to the enum. It's also
clearer
> that the function always returns a value -- I think MSVC is ok now with
> "default:", but with the above change it would sometimes warn defensively
about
> a function that may not return a value.
> 
> (more below)

Done. Note, I put
NOTREACHED();
return false; under case TEXT_DIRECTION_NUM_DIRECTIONS: because it needs
something under it.
Alternatively, I can just put a semicolon after case
TEXT_DIRECTION_NUM_DIRECTIONS:

https://codereview.chromium.org/2164483006/diff/580001/content/browser/render...
File content/browser/renderer_host/render_widget_host_view_mac.mm (right):

https://codereview.chromium.org/2164483006/diff/580001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_mac.mm:1101: //
TODO(spqchan): Change |selected_text_|'s type to string16.
On 2016/12/16 07:01:19, tapted wrote:
> I think changing |selected_text_| can be included -- it should be an easy
change
> :). The TODO should then go into OnGetRenderedTextCompleted (to change the
> |text| argument to string16)

Done.

https://codereview.chromium.org/2164483006/diff/580001/ui/views/controls/text...
File ui/views/controls/textfield/textfield.cc (right):

https://codereview.chromium.org/2164483006/diff/580001/ui/views/controls/text...
ui/views/controls/textfield/textfield.cc:1959: context_menu_contents_.reset();
On 2016/12/16 07:01:19, tapted wrote:
> this should have a comment

Done.

tapted

aw man this is tough. We can't depend on src/chrome from src/u There' doesn't seem ...

4 years ago (2016-12-19 07:41:21 UTC) #58

aw man this is tough. We can't depend on src/chrome from src/u

There' doesn't seem to be precedent for something like src/ui/ui_command_ids.h 
(we could then coordinate with chrome_comand_ids.h to ensure there's no overlap)
that might be something we need to explore though.

There are two other options below.

I think my preferred one at the moment is to just have RenderContextMenu and
TextfiledContextMenu handle the lookup word option themselves, and pass in
|index| to AppendToContextMenu (and call it Insert*)

https://codereview.chromium.org/2164483006/diff/600001/content/browser/render...
File content/browser/renderer_host/render_widget_host_view_mac.mm (right):

https://codereview.chromium.org/2164483006/diff/600001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_mac.mm:3392:
base::scoped_nsobject<NSString> text(SysUTF16ToNSString(str));
This might be a refcounting bug (nsobject expects a retained object, but
SysUTF16ToNSString will return an autoreleased one).

I think we can just get rid of |text|, and pass SysUTF16ToNSString(str) on to
the setString: arg in the return statement.

https://codereview.chromium.org/2164483006/diff/600001/ui/base/cocoa/DEPS
File ui/base/cocoa/DEPS (right):

https://codereview.chromium.org/2164483006/diff/600001/ui/base/cocoa/DEPS#new...
ui/base/cocoa/DEPS:2: "+chrome/app/chrome_command_ids.h",
ui/base can't depend on chrome. I think we will need to pass this in via the
delegate interface.

Perhaps IsLookUpAvailable() can take an int* for the command_id to use.

Then I think it's OK to keep using the IDS_ versions of the other commands (i.e.
those that appear in submenus rather than in the root level menu).

However, we can't use `IsTextServicesCommandId` the same way, since we can't
guarantee there's no overlap. However, this shouldn't be a big problem, since
it's just the lookup command that the root menu should see.

https://codereview.chromium.org/2164483006/diff/600001/ui/base/cocoa/text_ser...
File ui/base/cocoa/text_services_context_menu.cc (right):

https://codereview.chromium.org/2164483006/diff/600001/ui/base/cocoa/text_ser...
ui/base/cocoa/text_services_context_menu.cc:87: void
TextServicesContextMenu::AppendToContextMenu(SimpleMenuModel* model) {
we can perhaps just pass in |index|? (see below)

https://codereview.chromium.org/2164483006/diff/600001/ui/base/cocoa/text_ser...
ui/base/cocoa/text_services_context_menu.cc:96: int index =
model->GetIndexOfCommandId(IDC_SPELLCHECK_ADD_TO_DICTIONARY);
which means this is hard to fix (i.e. if we can't depend on chrome). 

Another thing to try might be to keep both RendererContextMenu and the
TextfieldContextMenu responsible for handling the "lookup" command.

i.e. TextServicesContextMenu only adds submenus.

I think this will remove the need for IsTextServicesCommand(..) too

spqchan

PTAL https://codereview.chromium.org/2164483006/diff/600001/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/2164483006/diff/600001/content/browser/renderer_host/render_widget_host_view_mac.mm#newcode3392 content/browser/renderer_host/render_widget_host_view_mac.mm:3392: base::scoped_nsobject<NSString> text(SysUTF16ToNSString(str)); On 2016/12/19 07:41:21, tapted wrote: > ...

4 years ago (2016-12-21 01:03:02 UTC) #59

PTAL

https://codereview.chromium.org/2164483006/diff/600001/content/browser/render...
File content/browser/renderer_host/render_widget_host_view_mac.mm (right):

https://codereview.chromium.org/2164483006/diff/600001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_mac.mm:3392:
base::scoped_nsobject<NSString> text(SysUTF16ToNSString(str));
On 2016/12/19 07:41:21, tapted wrote:
> This might be a refcounting bug (nsobject expects a retained object, but
> SysUTF16ToNSString will return an autoreleased one).
> 
> I think we can just get rid of |text|, and pass SysUTF16ToNSString(str) on to
> the setString: arg in the return statement.

Done.

https://codereview.chromium.org/2164483006/diff/600001/ui/base/cocoa/DEPS
File ui/base/cocoa/DEPS (right):

https://codereview.chromium.org/2164483006/diff/600001/ui/base/cocoa/DEPS#new...
ui/base/cocoa/DEPS:2: "+chrome/app/chrome_command_ids.h",
On 2016/12/19 07:41:21, tapted wrote:
> ui/base can't depend on chrome. I think we will need to pass this in via the
> delegate interface.
> 
> Perhaps IsLookUpAvailable() can take an int* for the command_id to use.
> 
> Then I think it's OK to keep using the IDS_ versions of the other commands
(i.e.
> those that appear in submenus rather than in the root level menu).
> 
> However, we can't use `IsTextServicesCommandId` the same way, since we can't
> guarantee there's no overlap. However, this shouldn't be a big problem, since
> it's just the lookup command that the root menu should see.

That works too. I think we should just take lookup out completely, since the
code would be a bit cleaner too

https://codereview.chromium.org/2164483006/diff/600001/ui/base/cocoa/text_ser...
File ui/base/cocoa/text_services_context_menu.cc (right):

https://codereview.chromium.org/2164483006/diff/600001/ui/base/cocoa/text_ser...
ui/base/cocoa/text_services_context_menu.cc:87: void
TextServicesContextMenu::AppendToContextMenu(SimpleMenuModel* model) {
On 2016/12/19 07:41:21, tapted wrote:
> we can perhaps just pass in |index|? (see below)

I think it's better to just get rid of look up from TextServicesContextMenu.

https://codereview.chromium.org/2164483006/diff/600001/ui/base/cocoa/text_ser...
ui/base/cocoa/text_services_context_menu.cc:96: int index =
model->GetIndexOfCommandId(IDC_SPELLCHECK_ADD_TO_DICTIONARY);
On 2016/12/19 07:41:21, tapted wrote:
> which means this is hard to fix (i.e. if we can't depend on chrome). 
> 
> Another thing to try might be to keep both RendererContextMenu and the
> TextfieldContextMenu responsible for handling the "lookup" command.
> 
> i.e. TextServicesContextMenu only adds submenus.
> 
> I think this will remove the need for IsTextServicesCommand(..) too
> 

Sounds good, let's just make it add only submenus

tapted

https://codereview.chromium.org/2164483006/diff/660001/chrome/browser/ui/cocoa/renderer_context_menu/render_view_context_menu_mac.mm File chrome/browser/ui/cocoa/renderer_context_menu/render_view_context_menu_mac.mm (right): https://codereview.chromium.org/2164483006/diff/660001/chrome/browser/ui/cocoa/renderer_context_menu/render_view_context_menu_mac.mm#newcode248 chrome/browser/ui/cocoa/renderer_context_menu/render_view_context_menu_mac.mm:248: if (text_services_context_menu_.IsTextServicesCommandId(command_id)) I think this isn't needed now (text ...

4 years ago (2016-12-21 11:20:28 UTC) #60

spqchan

The CQ bit was checked by spqchan@chromium.org to run a CQ dry run

4 years ago (2016-12-21 21:58:30 UTC) #61

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2164483006/680001

4 years ago (2016-12-21 21:59:05 UTC) #62

spqchan

I made some changes according to your review and is heading out for the holiday. ...

4 years ago (2016-12-21 22:00:14 UTC) #63

spqchan

I made some changes according to your review and is heading out for the holiday. ...

4 years ago (2016-12-21 22:00:18 UTC) #64

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

4 years ago (2016-12-21 22:02:50 UTC) #65

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-clang/builds/12183) ios-simulator-xcode-clang on ...

4 years ago (2016-12-21 22:02:51 UTC) #66

tapted

Have a great holiday! Yeah no rush - I'm away until Jan 3rd. https://codereview.chromium.org/2164483006/diff/660001/ui/base/cocoa/text_services_context_menu.cc File ...

4 years ago (2016-12-21 22:13:16 UTC) #67

Have a great holiday! Yeah no rush - I'm away until Jan 3rd.

https://codereview.chromium.org/2164483006/diff/660001/ui/base/cocoa/text_ser...
File ui/base/cocoa/text_services_context_menu.cc (right):

https://codereview.chromium.org/2164483006/diff/660001/ui/base/cocoa/text_ser...
ui/base/cocoa/text_services_context_menu.cc:20: SpeechChannel speechChannel;
On 2016/12/21 22:00:13, spqchan (OOO until Jan 9) wrote:
> On 2016/12/21 11:20:26, tapted wrote:
> > speechChannel -> g_speech_channel
> 
> Done. Quick question, why the g_?

This is to make it clear that we're declaring/referencing a global variable.
(i.e. even though it has internal linkage and isn't "global" in that sense, the
g_ makes it clear when referencing it that there is only one object). It's
commonly used (see e.g. `git grep '[^ ].* g_.*=.*;' -- '*.cc'`. There's no style
guideline for naming global variables, but this doesn't break any rules either
:)

https://codereview.chromium.org/2164483006/diff/660001/ui/base/cocoa/text_ser...
ui/base/cocoa/text_services_context_menu.cc:88: if
(printable_selection_text.empty())
On 2016/12/21 22:00:13, spqchan (OOO until Jan 9) wrote:
> On 2016/12/21 11:20:26, tapted wrote:
> > Hmmm - it looks like we can still speak when there is no selection, but only
> > when selecting from the Edit menu. But does this mean we need
> > OnSpeakRequested()? i.e. instead of
> > 
> > delegate_->OnSpeakRequested();
> > 
> > can we just do
> > 
> > SpeakText(delegate_->GetSelectedText())
> 
> We still need OnSpeakRequested() because if nothing is selected, then the host
> view will send an async request to retrieve all of the text.

But the logic here will not add the menu item if nothing is selected, so
OnSpeakRequested() _can't_ be called if nothing is selected :)

The async request only comes into play when selecting Edit->Speech->Start
Speaking. So we still need a public `SpeakText` so that -[RWHVC startSpeaking:]
can call it. However, the right-click context menu shouldn't need it.

spqchan

The CQ bit was checked by spqchan@chromium.org to run a CQ dry run

3 years, 11 months ago (2017-01-10 18:38:54 UTC) #68

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2164483006/700001

3 years, 11 months ago (2017-01-10 18:39:13 UTC) #69

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

3 years, 11 months ago (2017-01-10 18:41:36 UTC) #70

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds/133166) ios-device-xcode-clang on ...

3 years, 11 months ago (2017-01-10 18:41:37 UTC) #71

spqchan

The CQ bit was checked by spqchan@chromium.org to run a CQ dry run

3 years, 11 months ago (2017-01-10 23:22:42 UTC) #73

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2164483006/760001

3 years, 11 months ago (2017-01-10 23:23:44 UTC) #74

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

3 years, 11 months ago (2017-01-10 23:46:50 UTC) #75

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_rel_ng/builds/368285) mac_chromium_rel_ng on ...

3 years, 11 months ago (2017-01-10 23:46:52 UTC) #76

spqchan

The CQ bit was checked by spqchan@chromium.org to run a CQ dry run

3 years, 11 months ago (2017-01-11 19:02:30 UTC) #77

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2164483006/760001

3 years, 11 months ago (2017-01-11 19:03:00 UTC) #78

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

3 years, 11 months ago (2017-01-11 19:25:31 UTC) #79

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_compile_dbg_ng/builds/220300)

3 years, 11 months ago (2017-01-11 19:25:33 UTC) #80

spqchan

The CQ bit was checked by spqchan@chromium.org to run a CQ dry run

3 years, 11 months ago (2017-01-11 21:23:54 UTC) #81

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2164483006/780001

3 years, 11 months ago (2017-01-11 21:24:52 UTC) #82

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

3 years, 11 months ago (2017-01-11 21:44:34 UTC) #83

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_asan_rel_ng/builds/291270) linux_chromium_compile_dbg_ng on ...

3 years, 11 months ago (2017-01-11 21:44:35 UTC) #84

spqchan

The CQ bit was checked by spqchan@chromium.org to run a CQ dry run

3 years, 11 months ago (2017-01-11 22:19:45 UTC) #86

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2164483006/800001

3 years, 11 months ago (2017-01-11 22:20:51 UTC) #87

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

3 years, 11 months ago (2017-01-11 22:46:27 UTC) #88

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_ng/builds/367155)

3 years, 11 months ago (2017-01-11 22:46:29 UTC) #89

spqchan

The CQ bit was checked by spqchan@chromium.org to run a CQ dry run

3 years, 11 months ago (2017-01-11 23:56:53 UTC) #90

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2164483006/820001

3 years, 11 months ago (2017-01-11 23:58:47 UTC) #91

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

3 years, 11 months ago (2017-01-12 01:14:05 UTC) #92

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_ng/builds/367299)

3 years, 11 months ago (2017-01-12 01:14:07 UTC) #93

spqchan

The CQ bit was checked by spqchan@chromium.org to run a CQ dry run

3 years, 11 months ago (2017-01-12 19:03:45 UTC) #94

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2164483006/830001

3 years, 11 months ago (2017-01-12 19:04:38 UTC) #95

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

3 years, 11 months ago (2017-01-12 20:26:55 UTC) #96

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_ng/builds/367958)

3 years, 11 months ago (2017-01-12 20:26:56 UTC) #97

spqchan

The CQ bit was checked by spqchan@chromium.org to run a CQ dry run

3 years, 11 months ago (2017-01-12 23:47:52 UTC) #98

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2164483006/850001

3 years, 11 months ago (2017-01-12 23:49:07 UTC) #99

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

3 years, 11 months ago (2017-01-13 02:14:35 UTC) #100

commit-bot: I haz the power

Dry run: This issue passed the CQ dry run.

3 years, 11 months ago (2017-01-13 02:14:36 UTC) #101

spqchan

Rebased and fixed the tests. PTAL thanks :) https://codereview.chromium.org/2164483006/diff/660001/ui/base/cocoa/text_services_context_menu.cc File ui/base/cocoa/text_services_context_menu.cc (right): https://codereview.chromium.org/2164483006/diff/660001/ui/base/cocoa/text_services_context_menu.cc#newcode20 ui/base/cocoa/text_services_context_menu.cc:20: SpeechChannel ...

3 years, 11 months ago (2017-01-13 20:13:20 UTC) #102

Rebased and fixed the tests. PTAL thanks :)

https://codereview.chromium.org/2164483006/diff/660001/ui/base/cocoa/text_ser...
File ui/base/cocoa/text_services_context_menu.cc (right):

https://codereview.chromium.org/2164483006/diff/660001/ui/base/cocoa/text_ser...
ui/base/cocoa/text_services_context_menu.cc:20: SpeechChannel speechChannel;
On 2016/12/21 22:13:16, tapted wrote:
> On 2016/12/21 22:00:13, spqchan (OOO until Jan 9) wrote:
> > On 2016/12/21 11:20:26, tapted wrote:
> > > speechChannel -> g_speech_channel
> > 
> > Done. Quick question, why the g_?
> 
> This is to make it clear that we're declaring/referencing a global variable.
> (i.e. even though it has internal linkage and isn't "global" in that sense,
the
> g_ makes it clear when referencing it that there is only one object). It's
> commonly used (see e.g. `git grep '[^ ].* g_.*=.*;' -- '*.cc'`. There's no
style
> guideline for naming global variables, but this doesn't break any rules either
> :)

Gotcha, thanks!

https://codereview.chromium.org/2164483006/diff/660001/ui/base/cocoa/text_ser...
ui/base/cocoa/text_services_context_menu.cc:88: if
(printable_selection_text.empty())
On 2016/12/21 22:13:15, tapted wrote:
> On 2016/12/21 22:00:13, spqchan (OOO until Jan 9) wrote:
> > On 2016/12/21 11:20:26, tapted wrote:
> > > Hmmm - it looks like we can still speak when there is no selection, but
only
> > > when selecting from the Edit menu. But does this mean we need
> > > OnSpeakRequested()? i.e. instead of
> > > 
> > > delegate_->OnSpeakRequested();
> > > 
> > > can we just do
> > > 
> > > SpeakText(delegate_->GetSelectedText())
> > 
> > We still need OnSpeakRequested() because if nothing is selected, then the
host
> > view will send an async request to retrieve all of the text.
> 
> But the logic here will not add the menu item if nothing is selected, so
> OnSpeakRequested() _can't_ be called if nothing is selected :)
> 
> The async request only comes into play when selecting Edit->Speech->Start
> Speaking. So we still need a public `SpeakText` so that -[RWHVC
startSpeaking:]
> can call it. However, the right-click context menu shouldn't need it.

Doh, I see what you mean. Removed it :)

tapted

basically lg - worth looping in some OWNERS. Probably avi for the src/content stuff, and ...

3 years, 11 months ago (2017-01-13 21:37:33 UTC) #103

spqchan

The CQ bit was checked by spqchan@chromium.org to run a CQ dry run

3 years, 11 months ago (2017-01-25 00:53:00 UTC) #104

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2164483006/870001

3 years, 11 months ago (2017-01-25 00:54:06 UTC) #105

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

3 years, 11 months ago (2017-01-25 00:58:00 UTC) #106

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presubmit/builds/349400) ios-device on ...

3 years, 11 months ago (2017-01-25 00:58:02 UTC) #107

spqchan

The CQ bit was checked by spqchan@chromium.org to run a CQ dry run

3 years, 11 months ago (2017-01-26 01:44:26 UTC) #108

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2164483006/890001

3 years, 11 months ago (2017-01-26 01:45:03 UTC) #109

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

3 years, 11 months ago (2017-01-26 03:21:07 UTC) #110

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_swarming_rel/builds/107318)

3 years, 11 months ago (2017-01-26 03:21:08 UTC) #111

spqchan

spqchan@chromium.org changed reviewers: + asvitkine@chromium.org, msw@chromium.org, nasko@chromium.org

3 years, 11 months ago (2017-01-26 18:26:58 UTC) #112

spqchan

Thanks! Addressed your comments. It looks like avi is OOO until February + msw Please ...

3 years, 11 months ago (2017-01-26 18:27:01 UTC) #113

msw

mostly minor comments https://codereview.chromium.org/2164483006/diff/890001/ui/gfx/decorated_text_mac.h File ui/gfx/decorated_text_mac.h (right): https://codereview.chromium.org/2164483006/diff/890001/ui/gfx/decorated_text_mac.h#newcode16 ui/gfx/decorated_text_mac.h:16: // Returns a NSAttributedString from |decorated_text|. ...

3 years, 11 months ago (2017-01-26 20:59:16 UTC) #114

spqchan

The CQ bit was checked by spqchan@chromium.org to run a CQ dry run

3 years, 10 months ago (2017-01-31 01:14:53 UTC) #115

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2164483006/930001

3 years, 10 months ago (2017-01-31 01:15:20 UTC) #116

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

3 years, 10 months ago (2017-01-31 01:33:18 UTC) #117

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_chromium_compile_only_ng/builds/273633) linux_chromium_chromeos_ozone_rel_ng on ...

3 years, 10 months ago (2017-01-31 01:33:20 UTC) #118

spqchan

The CQ bit was checked by spqchan@chromium.org to run a CQ dry run

3 years, 10 months ago (2017-01-31 01:55:24 UTC) #119

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2164483006/950001

3 years, 10 months ago (2017-01-31 01:55:47 UTC) #120

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

3 years, 10 months ago (2017-01-31 03:35:05 UTC) #121

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_rel_ng/builds/380924)

3 years, 10 months ago (2017-01-31 03:35:07 UTC) #122

nasko

content/ changes seem mostly moving code around. I do have one question though. https://codereview.chromium.org/2164483006/diff/950001/content/browser/frame_host/render_widget_host_view_guest.cc File ...

3 years, 10 months ago (2017-01-31 15:37:29 UTC) #123

spqchan

The CQ bit was checked by spqchan@chromium.org to run a CQ dry run

3 years, 10 months ago (2017-01-31 18:14:48 UTC) #124

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2164483006/950001

3 years, 10 months ago (2017-01-31 18:15:30 UTC) #125

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

3 years, 10 months ago (2017-01-31 20:10:09 UTC) #126

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: linux_site_isolation on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_site_isolation/builds/5761)

3 years, 10 months ago (2017-01-31 20:10:11 UTC) #127

spqchan

The CQ bit was checked by spqchan@chromium.org to run a CQ dry run

3 years, 10 months ago (2017-01-31 22:23:13 UTC) #128

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2164483006/970001

3 years, 10 months ago (2017-01-31 22:24:12 UTC) #129

spqchan

PTAL, I added a test and addressed the comments from msw and nasko https://codereview.chromium.org/2164483006/diff/890001/ui/gfx/decorated_text_mac.h File ...

3 years, 10 months ago (2017-02-01 00:32:39 UTC) #130

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

3 years, 10 months ago (2017-02-01 00:35:05 UTC) #131

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: linux_site_isolation on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_site_isolation/builds/5771)

3 years, 10 months ago (2017-02-01 00:35:07 UTC) #132

msw

In the future, you may find it helpful (and some reviewers will too), if you ...

3 years, 10 months ago (2017-02-01 01:52:29 UTC) #133

Alexei Svitkine (slow)

https://codereview.chromium.org/2164483006/diff/970001/content/browser/renderer_host/render_widget_host_view_mac.h File content/browser/renderer_host/render_widget_host_view_mac.h (right): https://codereview.chromium.org/2164483006/diff/970001/content/browser/renderer_host/render_widget_host_view_mac.h#newcode485 content/browser/renderer_host/render_widget_host_view_mac.h:485: // TODO(spqchan): Change |text| to be the type string16. ...

3 years, 10 months ago (2017-02-01 19:12:38 UTC) #170

spqchan

The CQ bit was checked by spqchan@chromium.org to run a CQ dry run

3 years, 10 months ago (2017-02-02 02:35:13 UTC) #171

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2164483006/1010001

3 years, 10 months ago (2017-02-02 02:35:38 UTC) #173

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

3 years, 10 months ago (2017-02-02 03:44:35 UTC) #174

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_ng/builds/380297)

3 years, 10 months ago (2017-02-02 03:44:37 UTC) #175

spqchan

The CQ bit was checked by spqchan@chromium.org to run a CQ dry run

3 years, 10 months ago (2017-02-02 18:02:07 UTC) #177

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2164483006/1030001

3 years, 10 months ago (2017-02-02 18:02:46 UTC) #178

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

3 years, 10 months ago (2017-02-02 19:17:51 UTC) #179

commit-bot: I haz the power

Dry run: This issue passed the CQ dry run.

3 years, 10 months ago (2017-02-02 19:17:52 UTC) #180

spqchan

msw@, thanks of the heads up. I deleted the extra patches :) Addressed the comments ...

3 years, 10 months ago (2017-02-02 20:35:06 UTC) #181

Alexei Svitkine (slow)

https://codereview.chromium.org/2164483006/diff/970001/content/browser/renderer_host/render_widget_host_view_mac.h File content/browser/renderer_host/render_widget_host_view_mac.h (right): https://codereview.chromium.org/2164483006/diff/970001/content/browser/renderer_host/render_widget_host_view_mac.h#newcode485 content/browser/renderer_host/render_widget_host_view_mac.h:485: // TODO(spqchan): Change |text| to be the type string16. ...

3 years, 10 months ago (2017-02-02 20:50:24 UTC) #182

https://codereview.chromium.org/2164483006/diff/970001/content/browser/render...
File content/browser/renderer_host/render_widget_host_view_mac.h (right):

https://codereview.chromium.org/2164483006/diff/970001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_mac.h:485: //
TODO(spqchan): Change |text| to be the type string16.
On 2017/02/02 20:35:05, spqchan wrote:
> On 2017/02/01 19:12:37, Alexei Svitkine (very slow) wrote:
> > Can you provide more context here? Why is string16 preferable?
> > 
> > If it's to avoid some conversions - maybe the TODO should live where the
> > conversions happen?
> 
> This is done to avoid some conversions, since selected_text always get
converted
> from UTF16 to UTF8.
> Do you mean putting it in render_view_impl.cc? I kind of prefer putting it
here
> but I don't feel strongly about it so if you want me to move it there, I can
do
> that.

If you keep it here, expand the comment to refer to the conversion in question
so that it's clear *why*. (But yeah my suggestion was to put in .cc so it's
clear the conversion it's talking about.)

https://codereview.chromium.org/2164483006/diff/970001/ui/base/cocoa/text_ser...
File ui/base/cocoa/text_services_context_menu.cc (right):

https://codereview.chromium.org/2164483006/diff/970001/ui/base/cocoa/text_ser...
ui/base/cocoa/text_services_context_menu.cc:68: NewSpeechChannel(nullptr,
&g_speech_channel);
On 2017/02/02 20:35:06, spqchan wrote:
> Whoops, that's a good point. I added a check for it. Currently it just created
a
> new one but we can just reuse the old one
> I added a check for the OSErr return value too.
> 
> I added a TODO for a test suite. I'm not sure if I'll be able to write unit
> tests for the speech stuff but I can give it a try.

I would prefer it's done in this CL so that it's not forgotten.

I think it can be pretty basic to start with - e.g. exercising the functions to
make sure they don't crash.

For the context menu stuff, seems tests should be pretty easy too - check if
entries were added to the model, etc.

msw

https://codereview.chromium.org/2164483006/diff/950001/ui/gfx/render_text_unittest.cc File ui/gfx/render_text_unittest.cc (right): https://codereview.chromium.org/2164483006/diff/950001/ui/gfx/render_text_unittest.cc#newcode4021 ui/gfx/render_text_unittest.cc:4021: for (size_t i = 0; i < expected_text_1.text.length(); i++) ...

3 years, 10 months ago (2017-02-02 22:16:08 UTC) #183

https://codereview.chromium.org/2164483006/diff/950001/ui/gfx/render_text_uni...
File ui/gfx/render_text_unittest.cc (right):

https://codereview.chromium.org/2164483006/diff/950001/ui/gfx/render_text_uni...
ui/gfx/render_text_unittest.cc:4021: for (size_t i = 0; i <
expected_text_1.text.length(); i++) {
On 2017/02/02 20:35:05, spqchan wrote:
> On 2017/02/01 01:52:28, msw wrote:
> > Can you do this without a loop, like:
> > expected_text_1.attributes.push_back(CreateRangedAttribute(font_spans, 0,
> > expected_text_1.text.length(), Font::Weight::NORMAL, UNDERLINE_MASK));
> 
> No, it's weird like that. You need to apply an attribute to every character on
> the text.

There are numerous other places in the code that call CreateRangedAttribute once
with a non-empty range... why is this different?

https://codereview.chromium.org/2164483006/diff/1030001/ui/gfx/render_text_un...
File ui/gfx/render_text_unittest.cc (right):

https://codereview.chromium.org/2164483006/diff/1030001/ui/gfx/render_text_un...
ui/gfx/render_text_unittest.cc:4001: // Verify
GetDecoratedTextAndBaselineAtRange returns the correct baseline point
GetDecoratedTextAndBaselineForRange - please just make sure this matches the
name of the function you're testing...

https://codereview.chromium.org/2164483006/diff/1030001/ui/gfx/render_text_un...
ui/gfx/render_text_unittest.cc:4003: TEST_P(RenderTextHarfBuzzTest,
GetDecoratedTextAndBaselineAtRange) {
GetDecoratedTextAndBaselineForRange - please just make sure this matches the
name of the function you're testing...

https://codereview.chromium.org/2164483006/diff/1030001/ui/gfx/render_text_un...
ui/gfx/render_text_unittest.cc:4059:
expected_text_3.attributes.push_back(CreateRangedAttribute(
Can you build the attributes from the earlier words? eg:
expected_text_3.attributes.insert(expected_text_3.attributes.begin(),
expected_text_1.attributes.begin(), expected_text_1.attributes.end());

expected_text_3.attributes.push_back(CreateRangedAttribute(font_spans, 6, 6,
Font::Weight::NORMAL, STRIKE_MASK));

expected_text_3.attributes.insert(expected_text_3.attributes.end(),
expected_text_2.attributes.begin(), expected_text_2.attributes.end());

https://codereview.chromium.org/2164483006/diff/1030001/ui/gfx/render_text_un...
ui/gfx/render_text_unittest.cc:4071: const Rect left_glyph_3 =
render_text->GetCursorBounds(
nit: remove this and just use |left_glyph_1|?

tapted

lgtm - mostly just nits and a few thoughts https://codereview.chromium.org/2164483006/diff/1050001/content/browser/renderer_host/render_widget_host_view_mac.h File content/browser/renderer_host/render_widget_host_view_mac.h (right): https://codereview.chromium.org/2164483006/diff/1050001/content/browser/renderer_host/render_widget_host_view_mac.h#newcode518 content/browser/renderer_host/render_widget_host_view_mac.h:518: ...

3 years, 10 months ago (2017-02-03 10:14:51 UTC) #184

Alexei Svitkine (slow)

lgtm % comments https://codereview.chromium.org/2164483006/diff/1050001/chrome/browser/ui/cocoa/renderer_context_menu/render_view_context_menu_mac.h File chrome/browser/ui/cocoa/renderer_context_menu/render_view_context_menu_mac.h (right): https://codereview.chromium.org/2164483006/diff/1050001/chrome/browser/ui/cocoa/renderer_context_menu/render_view_context_menu_mac.h#newcode62 chrome/browser/ui/cocoa/renderer_context_menu/render_view_context_menu_mac.h:62: // Helper method that returns the ...

3 years, 10 months ago (2017-02-03 15:43:28 UTC) #185

Alexei Svitkine (slow)

lgtm % comments https://codereview.chromium.org/2164483006/diff/1050001/chrome/browser/ui/cocoa/renderer_context_menu/render_view_context_menu_mac.h File chrome/browser/ui/cocoa/renderer_context_menu/render_view_context_menu_mac.h (right): https://codereview.chromium.org/2164483006/diff/1050001/chrome/browser/ui/cocoa/renderer_context_menu/render_view_context_menu_mac.h#newcode62 chrome/browser/ui/cocoa/renderer_context_menu/render_view_context_menu_mac.h:62: // Helper method that returns the ...

3 years, 10 months ago (2017-02-03 15:43:34 UTC) #186

spqchan

The CQ bit was checked by spqchan@chromium.org to run a CQ dry run

3 years, 10 months ago (2017-02-03 21:14:58 UTC) #187

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2164483006/1050001

3 years, 10 months ago (2017-02-03 21:15:27 UTC) #188

spqchan

The CQ bit was checked by spqchan@chromium.org to run a CQ dry run

3 years, 10 months ago (2017-02-03 21:23:24 UTC) #189

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2164483006/1070001

3 years, 10 months ago (2017-02-03 21:23:56 UTC) #190

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

3 years, 10 months ago (2017-02-03 21:46:59 UTC) #191

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_compile_dbg_ng/builds/349508)

3 years, 10 months ago (2017-02-03 21:47:01 UTC) #192

spqchan

The CQ bit was checked by spqchan@chromium.org to run a CQ dry run

3 years, 10 months ago (2017-02-03 21:52:37 UTC) #193

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2164483006/1090001

3 years, 10 months ago (2017-02-03 21:53:26 UTC) #194

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

3 years, 10 months ago (2017-02-03 23:12:30 UTC) #195

commit-bot: I haz the power

Dry run: This issue passed the CQ dry run.

3 years, 10 months ago (2017-02-03 23:12:32 UTC) #196

spqchan

PTAL, addressed all of the comments. Let me know if I missed any, thanks! https://codereview.chromium.org/2164483006/diff/970001/content/browser/renderer_host/render_widget_host_view_mac.h ...

3 years, 10 months ago (2017-02-03 23:32:11 UTC) #198

PTAL, addressed all of the comments. Let me know if I missed any, thanks!

https://codereview.chromium.org/2164483006/diff/970001/content/browser/render...
File content/browser/renderer_host/render_widget_host_view_mac.h (right):

https://codereview.chromium.org/2164483006/diff/970001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_mac.h:485: //
TODO(spqchan): Change |text| to be the type string16.
On 2017/02/02 20:50:24, Alexei Svitkine (very slow) wrote:
> On 2017/02/02 20:35:05, spqchan wrote:
> > On 2017/02/01 19:12:37, Alexei Svitkine (very slow) wrote:
> > > Can you provide more context here? Why is string16 preferable?
> > > 
> > > If it's to avoid some conversions - maybe the TODO should live where the
> > > conversions happen?
> > 
> > This is done to avoid some conversions, since selected_text always get
> converted
> > from UTF16 to UTF8.
> > Do you mean putting it in render_view_impl.cc? I kind of prefer putting it
> here
> > but I don't feel strongly about it so if you want me to move it there, I can
> do
> > that.
> 
> If you keep it here, expand the comment to refer to the conversion in question
> so that it's clear *why*. (But yeah my suggestion was to put in .cc so it's
> clear the conversion it's talking about.)

Done.

https://codereview.chromium.org/2164483006/diff/970001/ui/base/cocoa/text_ser...
File ui/base/cocoa/text_services_context_menu.cc (right):

https://codereview.chromium.org/2164483006/diff/970001/ui/base/cocoa/text_ser...
ui/base/cocoa/text_services_context_menu.cc:68: NewSpeechChannel(nullptr,
&g_speech_channel);
On 2017/02/02 20:50:24, Alexei Svitkine (very slow) wrote:
> On 2017/02/02 20:35:06, spqchan wrote:
> > Whoops, that's a good point. I added a check for it. Currently it just
created
> a
> > new one but we can just reuse the old one
> > I added a check for the OSErr return value too.
> > 
> > I added a TODO for a test suite. I'm not sure if I'll be able to write unit
> > tests for the speech stuff but I can give it a try.
> 
> I would prefer it's done in this CL so that it's not forgotten.
> 
> I think it can be pretty basic to start with - e.g. exercising the functions
to
> make sure they don't crash.
> 
> For the context menu stuff, seems tests should be pretty easy too - check if
> entries were added to the model, etc.

Done.

https://codereview.chromium.org/2164483006/diff/1030001/ui/gfx/render_text_un...
File ui/gfx/render_text_unittest.cc (right):

https://codereview.chromium.org/2164483006/diff/1030001/ui/gfx/render_text_un...
ui/gfx/render_text_unittest.cc:4001: // Verify
GetDecoratedTextAndBaselineAtRange returns the correct baseline point
On 2017/02/02 22:16:08, msw wrote:
> GetDecoratedTextAndBaselineForRange - please just make sure this matches the
> name of the function you're testing...

I'm a bit confused. Isn't it matching?

https://codereview.chromium.org/2164483006/diff/1030001/ui/gfx/render_text_un...
ui/gfx/render_text_unittest.cc:4003: TEST_P(RenderTextHarfBuzzTest,
GetDecoratedTextAndBaselineAtRange) {
On 2017/02/02 22:16:08, msw wrote:
> GetDecoratedTextAndBaselineForRange - please just make sure this matches the
> name of the function you're testing...

Ditto from above

https://codereview.chromium.org/2164483006/diff/1030001/ui/gfx/render_text_un...
ui/gfx/render_text_unittest.cc:4059:
expected_text_3.attributes.push_back(CreateRangedAttribute(
On 2017/02/02 22:16:08, msw wrote:
> Can you build the attributes from the earlier words? eg:
> expected_text_3.attributes.insert(expected_text_3.attributes.begin(),
> expected_text_1.attributes.begin(), expected_text_1.attributes.end());
> 
> expected_text_3.attributes.push_back(CreateRangedAttribute(font_spans, 6, 6,
> Font::Weight::NORMAL, STRIKE_MASK));
> 
> expected_text_3.attributes.insert(expected_text_3.attributes.end(),
> expected_text_2.attributes.begin(), expected_text_2.attributes.end());

Sure thing, I can use it for expected_text_1, but not expected_text_2 since the
|range| value in expected_text_2's attributes start from 0, not 7.

https://codereview.chromium.org/2164483006/diff/1030001/ui/gfx/render_text_un...
ui/gfx/render_text_unittest.cc:4071: const Rect left_glyph_3 =
render_text->GetCursorBounds(
On 2017/02/02 22:16:08, msw wrote:
> nit: remove this and just use |left_glyph_1|?

Done.

https://codereview.chromium.org/2164483006/diff/1050001/chrome/browser/ui/coc...
File
chrome/browser/ui/cocoa/renderer_context_menu/render_view_context_menu_mac.h
(right):

https://codereview.chromium.org/2164483006/diff/1050001/chrome/browser/ui/coc...
chrome/browser/ui/cocoa/renderer_context_menu/render_view_context_menu_mac.h:62:
// Helper method that returns the BiDi parameter value for |direction|.
On 2017/02/03 15:43:25, Alexei Svitkine (very slow) wrote:
> Nit: No need to say "Helper method". Just "Returns the ..."
> 
> What's a BiDi parameter? Can this either explain what the returned int values
> mean or point to some other place in the codebase where this is defined?
> Especially since you use this as an bool in some calling logic and it's
entirely
> not obvious to me what the values are.

Done.

https://codereview.chromium.org/2164483006/diff/1050001/chrome/browser/ui/coc...
File
chrome/browser/ui/cocoa/renderer_context_menu/render_view_context_menu_mac.mm
(right):

https://codereview.chromium.org/2164483006/diff/1050001/chrome/browser/ui/coc...
chrome/browser/ui/cocoa/renderer_context_menu/render_view_context_menu_mac.mm:234:
content::RenderViewHost* view_host = GetRenderViewHost();
On 2017/02/03 15:43:27, Alexei Svitkine (very slow) wrote:
> Nit: Move this above line 242 right before it's used.

Done.

https://codereview.chromium.org/2164483006/diff/1050001/content/browser/rende...
File content/browser/renderer_host/render_widget_host_view_mac.h (right):

https://codereview.chromium.org/2164483006/diff/1050001/content/browser/rende...
content/browser/renderer_host/render_widget_host_view_mac.h:485: //
TODO(spqchan): Change |text| to be the type string16.
On 2017/02/03 15:43:30, Alexei Svitkine (very slow) wrote:
> Nit: Still my previous comment of giving more context here - or moving this to
> the .cc file.
> 
> The context here could be " in order to avoid a string conversion in Xyz()".

Done.

https://codereview.chromium.org/2164483006/diff/1050001/content/browser/rende...
content/browser/renderer_host/render_widget_host_view_mac.h:518: // selected
text on the renderer.
On 2017/02/03 10:14:51, tapted wrote:
> nit (while you're here) selected -> Selected

Done.

https://codereview.chromium.org/2164483006/diff/1050001/content/browser/rende...
content/browser/renderer_host/render_widget_host_view_mac.h:551: base::string16
selection_text_;
On 2017/02/03 10:14:51, tapted wrote:
> huh - this is new. A little confusing having |selected_text_| and
> |selection_text_| -- I think this is only text selected in an editable element
> (and from the TODO I guess it's going away).

Added a comment for it

https://codereview.chromium.org/2164483006/diff/1050001/content/browser/rende...
File content/browser/renderer_host/render_widget_host_view_mac_unittest.mm
(right):

https://codereview.chromium.org/2164483006/diff/1050001/content/browser/rende...
content/browser/renderer_host/render_widget_host_view_mac_unittest.mm:1349:
EXPECT_EQ(base::UTF8ToUTF16("world"), selected_text());
On 2017/02/03 10:14:51, tapted wrote:
> nit: ASCIIToUTF16, same below

Done.

https://codereview.chromium.org/2164483006/diff/1050001/ui/base/cocoa/text_se...
File ui/base/cocoa/text_services_context_menu.cc (right):

https://codereview.chromium.org/2164483006/diff/1050001/ui/base/cocoa/text_se...
ui/base/cocoa/text_services_context_menu.cc:71: DCHECK_EQ(0, result);
On 2017/02/03 10:14:51, tapted wrote:
> OSSTATUS_DCHECK(result == noErr, result);

Done.

https://codereview.chromium.org/2164483006/diff/1050001/ui/base/cocoa/text_se...
ui/base/cocoa/text_services_context_menu.cc:77: void
TextServicesContextMenu::StopSpeaking() {
On 2017/02/03 15:43:32, Alexei Svitkine (very slow) wrote:
> Add a DCHECK here that g_speech_channel isn't null?

Done.

https://codereview.chromium.org/2164483006/diff/1050001/ui/base/cocoa/text_se...
File ui/base/cocoa/text_services_context_menu_unittest.mm (right):

https://codereview.chromium.org/2164483006/diff/1050001/ui/base/cocoa/text_se...
ui/base/cocoa/text_services_context_menu_unittest.mm:12: #include
"ui/base/cocoa/text_services_context_menu.h"
On 2017/02/03 15:43:32, Alexei Svitkine (very slow) wrote:
> This should be the first include.

Done.

https://codereview.chromium.org/2164483006/diff/1050001/ui/base/cocoa/text_se...
ui/base/cocoa/text_services_context_menu_unittest.mm:49:
ui::TextServicesContextMenu menu_;
On 2017/02/03 10:14:51, tapted wrote:
> protected: before this

Done.

https://codereview.chromium.org/2164483006/diff/1050001/ui/base/cocoa/text_se...
ui/base/cocoa/text_services_context_menu_unittest.mm:54: };
On 2017/02/03 10:14:51, tapted wrote:
> nit:
> private:
>   DISALLOW_COPY_AND_ASSIGN(..)

Done.

https://codereview.chromium.org/2164483006/diff/1050001/ui/base/cocoa/text_se...
ui/base/cocoa/text_services_context_menu_unittest.mm:103:
menu_.SpeakText(base::UTF8ToUTF16("boxfish"));
On 2017/02/03 10:14:51, tapted wrote:
> nit: ASCIIToUTF16

Done.

https://codereview.chromium.org/2164483006/diff/1050001/ui/base/cocoa/text_se...
ui/base/cocoa/text_services_context_menu_unittest.mm:104:
EXPECT_TRUE(menu_.IsSpeaking());
On 2017/02/03 15:43:33, Alexei Svitkine (very slow) wrote:
> On 2017/02/03 10:14:51, tapted wrote:
> > It's likely this will be OK - I'm not sure how it will play with the test
> > parallelisation and things (e.g. race conditions). It theory, there might be
a
> > context switch away from the process before this, which stalls the process
> long
> > enough for the string to be spoken entirely. Hopefully that's unlikely
though.
> 
> Yeah - maybe worth making the text string a bit longer to minimize this
chance?
> e.g. a few paragraphs

Done.

https://codereview.chromium.org/2164483006/diff/1050001/ui/base/cocoa/text_se...
ui/base/cocoa/text_services_context_menu_unittest.mm:130:
menu_.SpeakText(base::UTF8ToUTF16("boxfish"));
On 2017/02/03 10:14:51, tapted wrote:
> ASCII
> 
> Also I think IsSpeaking() depends on global system state -- it doesn't just
use
> the speech channel. If this test and SpeechApi run simultaneously there could
be
> unexpected behaviour, but I don't think enough to make the test fail.

That's a good point. I removed SpeechAPI test since this one is covering it
anyway.

msw

lgtm with 'At' -> 'For' comment addressed. https://codereview.chromium.org/2164483006/diff/1030001/ui/gfx/render_text_unittest.cc File ui/gfx/render_text_unittest.cc (right): https://codereview.chromium.org/2164483006/diff/1030001/ui/gfx/render_text_unittest.cc#newcode4001 ui/gfx/render_text_unittest.cc:4001: // Verify ...

3 years, 10 months ago (2017-02-03 23:38:35 UTC) #199

spqchan

The CQ bit was checked by spqchan@chromium.org to run a CQ dry run

3 years, 10 months ago (2017-02-04 00:01:21 UTC) #200

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2164483006/1110001

3 years, 10 months ago (2017-02-04 00:01:44 UTC) #201

spqchan

On 2017/02/03 23:38:35, msw wrote: > lgtm with 'At' -> 'For' comment addressed. > > ...

3 years, 10 months ago (2017-02-04 00:02:29 UTC) #202

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

3 years, 10 months ago (2017-02-04 01:17:06 UTC) #203

commit-bot: I haz the power

Dry run: This issue passed the CQ dry run.

3 years, 10 months ago (2017-02-04 01:17:07 UTC) #204

spqchan

On 2017/02/06 19:23:15, nasko (very slow) wrote: > Rubberstamp LGTM on content/ bits. thanks!

3 years, 10 months ago (2017-02-06 19:37:11 UTC) #206

spqchan

The patchset sent to the CQ was uploaded after l-g-t-m from tapted@chromium.org, asvitkine@chromium.org, msw@chromium.org Link ...

3 years, 10 months ago (2017-02-06 19:39:41 UTC) #208

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2164483006/1110001

3 years, 10 months ago (2017-02-06 19:40:20 UTC) #209

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

3 years, 10 months ago (2017-02-06 19:48:30 UTC) #210

commit-bot: I haz the power

Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presubmit/builds/357671)

3 years, 10 months ago (2017-02-06 19:48:32 UTC) #211

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2164483006/1110001

3 years, 10 months ago (2017-02-06 22:06:43 UTC) #213

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

3 years, 10 months ago (2017-02-06 22:19:09 UTC) #214

commit-bot: I haz the power

Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presubmit/builds/357869)

3 years, 10 months ago (2017-02-06 22:19:13 UTC) #215

spqchan

spqchan@chromium.org changed reviewers: + sky@chromium.org

3 years, 10 months ago (2017-02-06 22:32:40 UTC) #216

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2164483006/1110001

3 years, 10 months ago (2017-02-07 00:03:05 UTC) #221

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

3 years, 10 months ago (2017-02-07 00:10:14 UTC) #222

commit-bot: I haz the power

Failed to apply patch for content/browser/frame_host/render_widget_host_view_child_frame.h: While running git apply --index -p1; error: patch failed: ...

3 years, 10 months ago (2017-02-07 00:10:26 UTC) #223

spqchan

The patchset sent to the CQ was uploaded after l-g-t-m from tapted@chromium.org, asvitkine@chromium.org, sky@chromium.org, nasko@chromium.org, ...

3 years, 10 months ago (2017-02-07 01:03:52 UTC) #225

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2164483006/1130001

3 years, 10 months ago (2017-02-07 01:04:35 UTC) #226

commit-bot: I haz the power

CQ is committing da patch. Bot data: {"patchset_id": 1130001, "attempt_start_ts": 1486429432045330, "parent_rev": "2f2e19599e1d1658516378245c256456318392c9", "commit_rev": "375ddabe9cdc45673e112c5b1b06cc754aa45759"}

3 years, 10 months ago (2017-02-07 02:41:34 UTC) #227

commit-bot: I haz the power

Description was changed from ========== [MacViews] Implemented text context menu - Added Speech, Writing Direction ...

3 years, 10 months ago (2017-02-07 02:42:41 UTC) #228

commit-bot: I haz the power

3 years, 10 months ago (2017-02-07 02:42:43 UTC) #229

Message was sent while issue was closed.

Committed patchset #15 (id:1130001) as
https://chromium.googlesource.com/chromium/src/+/375ddabe9cdc45673e112c5b1b06...

Issue 2164483006: [MacViews] Implemented text context menu (Closed)

Description

Patch Set 1 #

Patch Set 2 : ditto #

Patch Set 3 : Addressed tapted's comments and made things work #

Patch Set 4 : Fix for tapted #

Patch Set 5 : Fix for tapted 2 #

Patch Set 6 : Fix for tapted 3 #

Patch Set 7 : Addressed tapted's comments and fixed tests #

Patch Set 8 : Addressed tapted's comments and added a test #

Patch Set 9 : Addressed msw's comments, changed 2016 to 2017 #

Patch Set 10 : Rebased #

Patch Set 11 : Addressed comments #

Patch Set 12 : Added test suite for TextServicesContextMenu #

Patch Set 13 : Addressed comments #

Patch Set 14 : At->For #

Patch Set 15 : rebased #

Messages