1261299 - [e10s] Mac OS X Services is not available for e10s tab content

Status: RESOLVED FIXED

(Core :: Widget: Cocoa, defect, P1)

Description

[Tim Guan-tin Chien [:timdream] (please needinfo)]

STR:

1. With e10s on, select any text from a web page
2. Inspect the Services available by click the top-right Firefox > Services

Expected result:

1. Be able to access Mac OS X Services, like "Speak Text"

Actual result:

1. A gray out items says "No Services Apply".

Note: This is e10s related because selected text in non-e10s page like Preferences tab is not affected.

Comment 1

[Jim Mathies [:jimm]]

Hey felipe, do you know if this is a11y related?

Comment 2

[:Felipe Gomes (needinfo for replies!)]

This is not related to bug 1260190 that landed recently. I checked and this is not a new bug, it has always existed with e10s.

Now, I don't know if this "Services" feature uses the OSX's a11y APIs.. I suspect it might. But I don't know about OSX system programming to be able to answer that.

Comment 3

[Neil Deakin]

It doesn't relate to accessibility. This is done in nsChildView.mm validRequestorForSendType and writeSelectionToPasteboard. An WidgetQueryContentEvent event is sent which gets the selection. Presumably, we don't forward this to the child process, but I'm not sure if this will work asyncronously.

Comment 4

[Jim Mathies [:jimm]]

Jeff, should this block?

Comment 5

[Jeff Griffiths (:canuckistani) (:⚡︎)]

(In reply to Jim Mathies [:jimm] from comment #4)
> Jeff, should this block?

Yes, I think so.

Comment 6

[Gabor Krizsanits (INACTIVE)]

I don't think this is the most popular feature of the world, but I do think it would look bad on us if we broke it... I kind of expect this feature to work to some extent from a serious application on OSX so I kind of agree with the blocking.

(In reply to Neil Deakin from comment #3)
> It doesn't relate to accessibility. This is done in nsChildView.mm
> validRequestorForSendType and writeSelectionToPasteboard. An
> WidgetQueryContentEvent event is sent which gets the selection. Presumably,
> we don't forward this to the child process, but I'm not sure if this will
> work asyncronously.

The docs are a bit vague but I think it will not. But I really see no reason why we could not do a quick sync messaging to get a bool from content (isTextSelected) and then implement writeSelectionToPasteboard in a way that it only gets the selected text when a service requests it, then we could do another sync message for the text.

For the first message we could even use the push scheme, so every time text is selected we notify the parent about it, but I don't think this will ever become a performance issue so I would not bother. For the actual text query I would prefer a tiny little lag when someone initiates a service with a huge text than spamming the IPC channels with selected texts all the time for a rarely used feature.

Comment 7

[Mike Conley (:mconley) (:⚙️)]

One thing we might want to try here is adding support for dynamic menupopup population for OS X native menus. That way, we could send an async message down to content to get at the current selection and once the result comes back, populate the Services menu dynamically once the result comes back.

I don't believe the OS X native menu support we have in widget/cocoa supports adding menuitems after the native popup is opened, and I think we'd need to add that support.

Comment 8

[Jimmy Wang (:jimicy) - works on e10s stuff]

Masayuki said as of right now
The IPC sync message NotifyIMEFocus from content process is sent only while editable content (text area for example) has focus. And ContentCache is available only when an editable content has focus
http://dxr.mozilla.org/mozilla-central/source/dom/ipc/PBrowser.ipdl#199
So, when you select text outside editors, you don't have any way to retrieve the selected contents.

How can I send a text selection up from the content to the parent?
What files should I look in to add such a functionality?
So would I send an IPC sync message from parent telling content to send up the text selection?

Comment 9

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to Jimmy Wang (:jimicy) - works on e10s stuff from comment #8)
> How can I send a text selection up from the content to the parent?

It depends on how is OS X's API designed. If it simply needs the result synchronously, we need to cache selection even if no editors have focus.  Otherwise, i.e., like Dictionary Service, you can query the selection with async IPC.

> What files should I look in to add such a functionality?

If you need to cache selection in the parent (this is bad scenario...), you should create IMEContentObserver even if no editors have focus. But in such case, IMEContentObserver should listen only selection change and notify parent process of only selection changes.

> So would I send an IPC sync message from parent telling content to send up
> the text selection?

No, you cannot. In strictly speaking, it's possible technically but it's not allowed for avoiding chrome process to hang up.

Comment 10

[Makoto Kato [:m_kato]]

I think that you should use nsISelectionListener on content process.  Then if notifySelectionChanged is called, send calculated hasTextSelection to chrome process via IPC or message.  When no focus, use it.

Comment 11

[Reuben Morais [:reuben]]

Attachment: Very hacky workaround

Last weekend I finally got frustrated enough about Open-in-Bugzilla not working in Firefox to look into this and write this very hacky workaround. It only works when there's a focused editable content, and it only sends plain text. This is probably not very useful but here it is anyway, if anyone wants a local fix.

Comment 12

[Jimmy Wang (:jimicy) - works on e10s stuff]

Attachment: Bug 1261299 - Add new clipboard kSelectionCache to cache the current selection for OSX service menu. Add a constructor to nsAutoCopyListener which sets the clipboard to copy to. Pass in kSelectionCache for OSX or kSelectionClipboard for linux.

Review commit: https://reviewboard.mozilla.org/r/63202/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/63202/

Comment 13

[Jimmy Wang (:jimicy) - works on e10s stuff]

Attachment: Bug 1261299 - Add sSelectionTransferable and to get the current selection (chrome/content) needed for the OSX service menu.

***
Bug 1261299 - Add sSelectionTransferable and use in e10s to get the content selection needed for the OSX service menu.

Review commit: https://reviewboard.mozilla.org/r/63204/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/63204/

Comment 14

[Jimmy Wang (:jimicy) - works on e10s stuff]

Attachment: Bug 1261299 - Add method nsCopySupport::ClearSelectionTransferable() to clear the nsClipboard::sSelectionTransferable when you have no content selection.

Review commit: https://reviewboard.mozilla.org/r/63206/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/63206/

Comment 15

[Jimmy Wang (:jimicy) - works on e10s stuff]

Attachment: Bug 1261299 - Add a flag to check whether the widget query event is dispatched on the chrome or content process.

Review commit: https://reviewboard.mozilla.org/r/63208/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/63208/

Comment 16

[Jimmy Wang (:jimicy) - works on e10s stuff]

Attachment: Bug 1261299 - Add testing method GetSelectionAsPlaintext to DomWindowUtils which returns the text selection that was sent to the osx service menu nsChildView.mm.

Review commit: https://reviewboard.mozilla.org/r/63210/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/63210/

Comment 17

[Jimmy Wang (:jimicy) - works on e10s stuff]

Attachment: Bug 1261299 - Add test for checking that the selected content/chrome text is properly sent up to the osx service menu code (nsChildView.mm).

Review commit: https://reviewboard.mozilla.org/r/63212/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/63212/

Comment 18

[Jimmy Wang (:jimicy) - works on e10s stuff]

Attachment: Bug 1261299 - On e10s in nsChildView for OSX services, determine if there is a current selection (chrome/content). Send the selection to osx service menu.

Review commit: https://reviewboard.mozilla.org/r/63214/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/63214/

Comment 19

[Jimmy Wang (:jimicy) - works on e10s stuff]

Comment on attachment 8769244 [details]
Bug 1261299 - Add new clipboard kSelectionCache to cache the current selection for OSX service menu. Add a constructor to nsAutoCopyListener which sets the clipboard to copy to. Pass in kSelectionCache for OSX or kSelectionClipboard for linux.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63202/diff/1-2/

Comment 20

[Jimmy Wang (:jimicy) - works on e10s stuff]

Comment on attachment 8769245 [details]
Bug 1261299 - Add sSelectionTransferable and to get the current selection (chrome/content) needed for the OSX service menu.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63204/diff/1-2/

Comment 21

[Jimmy Wang (:jimicy) - works on e10s stuff]

Comment on attachment 8769246 [details]
Bug 1261299 - Add method nsCopySupport::ClearSelectionTransferable() to clear the nsClipboard::sSelectionTransferable when you have no content selection.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63206/diff/1-2/

Comment 22

[Jimmy Wang (:jimicy) - works on e10s stuff]

Comment on attachment 8769247 [details]
Bug 1261299 - Add a flag to check whether the widget query event is dispatched on the chrome or content process.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63208/diff/1-2/

Comment 23

[Jimmy Wang (:jimicy) - works on e10s stuff]

Comment on attachment 8769248 [details]
Bug 1261299 - Add testing method GetSelectionAsPlaintext to DomWindowUtils which returns the text selection that was sent to the osx service menu nsChildView.mm.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63210/diff/1-2/

Comment 24

[Jimmy Wang (:jimicy) - works on e10s stuff]

Comment on attachment 8769249 [details]
Bug 1261299 - Add test for checking that the selected content/chrome text is properly sent up to the osx service menu code (nsChildView.mm).

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63212/diff/1-2/

Comment 25

[Jimmy Wang (:jimicy) - works on e10s stuff]

Comment on attachment 8769250 [details]
Bug 1261299 - On e10s in nsChildView for OSX services, determine if there is a current selection (chrome/content). Send the selection to osx service menu.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63214/diff/1-2/

Comment 26

[Jimmy Wang (:jimicy) - works on e10s stuff]

Attachment: Bug 1261299 - Add EnsureDocument(mPresContext) to EventStateManager::GetFocusedContent() because mDocument is lazily loaded.

Review commit: https://reviewboard.mozilla.org/r/64144/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/64144/

Comment 27

[Jimmy Wang (:jimicy) - works on e10s stuff]

Comment on attachment 8769244 [details]
Bug 1261299 - Add new clipboard kSelectionCache to cache the current selection for OSX service menu. Add a constructor to nsAutoCopyListener which sets the clipboard to copy to. Pass in kSelectionCache for OSX or kSelectionClipboard for linux.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63202/diff/2-3/

Comment 28

[Jimmy Wang (:jimicy) - works on e10s stuff]

Comment on attachment 8769245 [details]
Bug 1261299 - Add sSelectionTransferable and to get the current selection (chrome/content) needed for the OSX service menu.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63204/diff/2-3/

Comment 29

[Jimmy Wang (:jimicy) - works on e10s stuff]

Comment on attachment 8769246 [details]
Bug 1261299 - Add method nsCopySupport::ClearSelectionTransferable() to clear the nsClipboard::sSelectionTransferable when you have no content selection.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63206/diff/2-3/

Comment 30

[Jimmy Wang (:jimicy) - works on e10s stuff]

Comment on attachment 8769247 [details]
Bug 1261299 - Add a flag to check whether the widget query event is dispatched on the chrome or content process.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63208/diff/2-3/

Comment 31

[Jimmy Wang (:jimicy) - works on e10s stuff]

Comment on attachment 8769248 [details]
Bug 1261299 - Add testing method GetSelectionAsPlaintext to DomWindowUtils which returns the text selection that was sent to the osx service menu nsChildView.mm.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63210/diff/2-3/

Comment 32

[Jimmy Wang (:jimicy) - works on e10s stuff]

Comment on attachment 8769249 [details]
Bug 1261299 - Add test for checking that the selected content/chrome text is properly sent up to the osx service menu code (nsChildView.mm).

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63212/diff/2-3/

Comment 33

[Jimmy Wang (:jimicy) - works on e10s stuff]

Comment on attachment 8769250 [details]
Bug 1261299 - On e10s in nsChildView for OSX services, determine if there is a current selection (chrome/content). Send the selection to osx service menu.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63214/diff/2-3/

Comment 34

[Jimmy Wang (:jimicy) - works on e10s stuff]

Comment on attachment 8769248 [details]
Bug 1261299 - Add testing method GetSelectionAsPlaintext to DomWindowUtils which returns the text selection that was sent to the osx service menu nsChildView.mm.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63210/diff/3-4/

Comment 35

[Jimmy Wang (:jimicy) - works on e10s stuff]

Comment on attachment 8769249 [details]
Bug 1261299 - Add test for checking that the selected content/chrome text is properly sent up to the osx service menu code (nsChildView.mm).

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63212/diff/3-4/

Comment 36

[Jimmy Wang (:jimicy) - works on e10s stuff]

Comment on attachment 8769250 [details]
Bug 1261299 - On e10s in nsChildView for OSX services, determine if there is a current selection (chrome/content). Send the selection to osx service menu.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63214/diff/3-4/

Comment 37

[Jimmy Wang (:jimicy) - works on e10s stuff]

Comment on attachment 8770813 [details]
Bug 1261299 - Add EnsureDocument(mPresContext) to EventStateManager::GetFocusedContent() because mDocument is lazily loaded.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64144/diff/1-2/

Comment 38

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8769248 [details]
Bug 1261299 - Add testing method GetSelectionAsPlaintext to DomWindowUtils which returns the text selection that was sent to the osx service menu nsChildView.mm.

https://reviewboard.mozilla.org/r/63210/#review61420

::: dom/base/nsDOMWindowUtils.cpp:1243
(Diff revision 4)
> +  if (!widget)
> +    return NS_ERROR_FAILURE;

nit: use {} even if the |if| block has only one line.

::: dom/interfaces/base/nsIDOMWindowUtils.idl:782
(Diff revision 4)
>     * Will throw a DOM security error if called without chrome privileges.
>     */
>    void forceUpdateNativeMenuAt(in AString indexString);
>  
>    /**
> +   * See nsIWidget::TestSendTextSelectionToServices

It doesn't make sense to refer internal header from exposed idl file.

::: dom/interfaces/base/nsIDOMWindowUtils.idl:784
(Diff revision 4)
>    void forceUpdateNativeMenuAt(in AString indexString);
>  
>    /**
> +   * See nsIWidget::TestSendTextSelectionToServices
> +   */
> +  AString TestSendTextSelectionToServices();

And I don't like this method name, see the last issue.

::: widget/cocoa/nsChildView.mm:6015
(Diff revision 4)
> +NS_IMETHODIMP nsChildView::TestSendTextSelectionToServices(nsAString& aResult)
> +{

I think that you need to wrap this method with NS_OBJC_BEGIN_TRY_ABORT_BLOCK_NSRESULT; and NS_OBJC_END_TRY_ABORT_BLOCK_NSRESULT;.

::: widget/cocoa/nsChildView.mm:6019
(Diff revision 4)
>  
> +NS_IMETHODIMP nsChildView::TestSendTextSelectionToServices(nsAString& aResult)
> +{
> +  WidgetQueryContentEvent event(true, eQuerySelectionAsTransferable, this);
> +  // This might destroy our widget (and null out mGeckoChild).
> +  this->DispatchWindowEvent(event);

|this->| isn't necessary. And please check Destroyed() after this call.

::: widget/cocoa/nsChildView.mm:6022
(Diff revision 4)
> +  WidgetQueryContentEvent event(true, eQuerySelectionAsTransferable, this);
> +  // This might destroy our widget (and null out mGeckoChild).
> +  this->DispatchWindowEvent(event);
> +
> +  // Transform the transferable to an NSDictionary.
> +  NSDictionary* pasteboardOutputDict;

Don't you need to initialize this with nullptr? According to the following code, this may be referred without being initialized.

::: widget/cocoa/nsChildView.mm:6024
(Diff revision 4)
> +  // On E10s, if there is a current chrome selection.
> +  if (!event.mIsContentProcess && event.mSucceeded && event.mReply.mHasSelection) {
> +    pasteboardOutputDict = nsClipboard::PasteboardDictFromTransferable(event.mReply.mTransferable);
> +  }
> +  // Otherwise on E10s, if there is a current content selection.
> +  else if (event.mIsContentProcess && nsClipboard::sSelectionTransferable) {
> +    pasteboardOutputDict = nsClipboard::PasteboardDictFromTransferable(nsClipboard::sSelectionTransferable);
> +  }

Looks odd to me.

First condition checks |!event.mIsContentProcess| but following |else if| condition checks |event.mIsContentProcess| but both comment says |on E10s|.

And please keep 80 characters per line rule here.


nit: s/E10s/e10s

::: widget/cocoa/nsChildView.mm:6044
(Diff revision 4)
> +  NSMutableArray* declaredTypes = [NSMutableArray arrayWithCapacity:typeCount];
> +  [declaredTypes addObjectsFromArray:[pasteboardOutputDict allKeys]];
> +  NSString* currentKey = [declaredTypes objectAtIndex:0];
> +  NSString* currentValue = [pasteboardOutputDict valueForKey:currentKey];
> +
> +  const char *selectedTextUTF8 = [currentValue UTF8String];

nit: cosnt char* selectedUTF8Text =...

(position of |*| and the name)

::: widget/cocoa/nsChildView.mm:6045
(Diff revision 4)
> +  AppendUTF8toUTF16(selectedTextUTF8, aResult);
> +  return NS_OK;

I think that the name of this method and what this method does don't match.

This method returns selected text from eQuerySelectionAsTransferable event or nsClipboard::sSelectionTransferable (I don't know why we cannot use the latter every time, though).

(Note that when you don't need to dispatch WidgetQueryContentEvent, you shouldn't do that because it may be very expensive. You can check if focused content is in remote process with InputContext::mOrigin, that is set by SetInputContext() and it's always correct on focused widget.

::: widget/nsBaseWidget.h:225
(Diff revision 4)
>                                            int32_t aHorizontal,
>                                            int32_t aVertical) override;
>    NS_IMETHOD              BeginMoveDrag(mozilla::WidgetMouseEvent* aEvent) override;
>    virtual nsresult        ActivateNativeMenuItemAt(const nsAString& indexString) override { return NS_ERROR_NOT_IMPLEMENTED; }
>    virtual nsresult        ForceUpdateNativeMenuAt(const nsAString& indexString) override { return NS_ERROR_NOT_IMPLEMENTED; }
> +  virtual nsresult        TestSendTextSelectionToServices(nsAString& aResult) override { return NS_ERROR_NOT_IMPLEMENTED; }

You can implement it as this in nsIWidget.h directly.

::: widget/nsIWidget.h:1701
(Diff revision 4)
>       * menu system.
>       */
>      virtual nsresult ForceUpdateNativeMenuAt(const nsAString& indexString) = 0;
>  
>      /**
> +     * See Bug 1261299

You shouldn't include bug number. Instead, you should explain about the method enough.

::: widget/nsIWidget.h:1702
(Diff revision 4)
>       */
>      virtual nsresult ForceUpdateNativeMenuAt(const nsAString& indexString) = 0;
>  
>      /**
> +     * See Bug 1261299
> +     * This is used for osx service menu testing.

s/osx/macOS

Comment 39

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8769247 [details]
Bug 1261299 - Add a flag to check whether the widget query event is dispatched on the chrome or content process.

https://reviewboard.mozilla.org/r/63208/#review61430

As I said in <https://reviewboard.mozilla.org/r/63210/#issue-summary>, I think that you can retrieve if focused content is in remote process with InputContext::mOrigin. So, this patch must not be necessary.

Comment 40

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8769249 [details]
Bug 1261299 - Add test for checking that the selected content/chrome text is properly sent up to the osx service menu code (nsChildView.mm).

https://reviewboard.mozilla.org/r/63212/#review61432

Nice, except some nits.

::: browser/base/content/test/general/browser.ini:220
(Diff revision 4)
>  [browser_bug565575.js]
>  [browser_bug567306.js]
>  subsuite = clipboard
> +[browser_bug1261299.js]
> +subsuite = clipboard
> +skip-if = toolkit != "cocoa" # Service menu is only supported on OSX. Bug 1261299

I think that the comment should be something like:

# Because of tests for supporting Service Menu of macOS, bug 1261299

::: browser/base/content/test/general/browser_bug1261299.js:22
(Diff revision 4)
> +  let DOMWindowUtils = EventUtils._getDOMWindowUtils(window);
> +  let selectedText;
> +
> +  yield BrowserTestUtils.openNewForegroundTab(gBrowser, testPage);
> +  yield BrowserTestUtils.synthesizeMouse("#textarea", 0, 0, {}, gBrowser.selectedBrowser);
> +  yield BrowserTestUtils.synthesizeKey("VK_RIGHT", {shiftKey: true, ctrlKey: true}, gBrowser.selectedBrowser);

Could you use "KEY_ArrowRight" instead of "VK_RIGHT" and add |code: "ArrowRight"| to the second argument? It emulates physical ArrowRight key more exactly.

::: browser/base/content/test/general/browser_bug1261299.js:23
(Diff revision 4)
> +  let selectedText;
> +
> +  yield BrowserTestUtils.openNewForegroundTab(gBrowser, testPage);
> +  yield BrowserTestUtils.synthesizeMouse("#textarea", 0, 0, {}, gBrowser.selectedBrowser);
> +  yield BrowserTestUtils.synthesizeKey("VK_RIGHT", {shiftKey: true, ctrlKey: true}, gBrowser.selectedBrowser);
> +  selectedText = DOMWindowUtils.TestSendTextSelectionToServices();

I don't like the method name, though.

::: browser/base/content/test/general/browser_bug1261299.js:24
(Diff revision 4)
> +
> +  yield BrowserTestUtils.openNewForegroundTab(gBrowser, testPage);
> +  yield BrowserTestUtils.synthesizeMouse("#textarea", 0, 0, {}, gBrowser.selectedBrowser);
> +  yield BrowserTestUtils.synthesizeKey("VK_RIGHT", {shiftKey: true, ctrlKey: true}, gBrowser.selectedBrowser);
> +  selectedText = DOMWindowUtils.TestSendTextSelectionToServices();
> +  is(selectedText, "Write something here", "The osx service (nsChildView.mm) got the selected content text");

nit: You should remove |(nsChildView.mm)| because the implementation might be moved to different file in the future, but I bet in such time, nobody realize that this should be fixed too.

::: browser/base/content/test/general/browser_bug1261299.js:28
(Diff revision 4)
> +  yield BrowserTestUtils.synthesizeKey("VK_RIGHT", {shiftKey: true, ctrlKey: true}, gBrowser.selectedBrowser);
> +  selectedText = DOMWindowUtils.TestSendTextSelectionToServices();
> +  is(selectedText, "test.mozilla.org", "The osx service (nsChildView.mm) got the selected chrome text");

Same above.

Comment 41

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://reviewboard.mozilla.org/r/63214/#review61434

Although, I don't familiar with macOS specific code, but looks fine to me.

::: widget/cocoa/nsChildView.mm:5902
(Diff revision 4)
> +        // On non-E10s, if there is no current selection.
> +        // On E10s, if there is no current chrome selection.

nit: s/E10s/e10s

But perhaps, you should explain with "if focused content is in a remote process", it must be easier to understand.

::: widget/cocoa/nsChildView.mm:5904
(Diff revision 4)
>          // This might destroy our widget (and null out mGeckoChild).
>          mGeckoChild->DispatchWindowEvent(event);
> -        if (!mGeckoChild || !event.mSucceeded || !event.mReply.mHasSelection)
> +
> +        // On non-E10s, if there is no current selection.
> +        // On E10s, if there is no current chrome selection.
> +        if (!event.mIsContentProcess && (!mGeckoChild ||

So, you should be able to check if focused content is in remote with InputContext::mOrigin.

And moving |(!mGeckoChild ||| to the next line must be easier to read.

::: widget/cocoa/nsChildView.mm:5908
(Diff revision 4)
> +        // On E10s, if there is no current chrome selection.
> +        if (!event.mIsContentProcess && (!mGeckoChild ||
> +            !event.mSucceeded || !event.mReply.mHasSelection)) {
> +          result = nil;
> +        }
> +        // Otherwise on E10s, if there is no current content selection.

nit: s/E10s/e10s

::: widget/cocoa/nsChildView.mm:5960
(Diff revision 4)
> +  if (event.mIsContentProcess) {
> +    pasteboardOutputDict = nsClipboard::PasteboardDictFromTransferable(nsClipboard::sSelectionTransferable);
> +  } else {
> +    pasteboardOutputDict = nsClipboard::PasteboardDictFromTransferable(event.mReply.mTransferable);
> +  }

nit: Looks like to long lines, please wrap with our coding rules..

Comment 42

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8769250 [details]
Bug 1261299 - On e10s in nsChildView for OSX services, determine if there is a current selection (chrome/content). Send the selection to osx service menu.

https://reviewboard.mozilla.org/r/63214/#review61436

Oops, forgot to mark this as r+...

Comment 43

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://reviewboard.mozilla.org/r/64144/#review61446

Looks good to me, but I'd like smaug to check about this since I don't understand why this is now a problem even though ESM is very old module.

Comment 44

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://reviewboard.mozilla.org/r/63210/#review61448

::: dom/interfaces/base/nsIDOMWindowUtils.idl:782
(Diff revision 4)
> +   * See nsIWidget::TestSendTextSelectionToServices
> +   */
> +  AString TestSendTextSelectionToServices();

And also I think that you should document that this method works only in the chrome process.

Comment 45

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8770813 [details]
Bug 1261299 - Add EnsureDocument(mPresContext) to EventStateManager::GetFocusedContent() because mDocument is lazily loaded.

https://reviewboard.mozilla.org/r/64144/#review61572

This is fine to me. The laziness of mDocument setup is a bit silly. Old stuff.

But could you explain where/why this is needed. What is the stack trace when GetFocusedContent() is called and mDocument is null?

Comment 46

[Jimmy Wang (:jimicy) - works on e10s stuff]

https://reviewboard.mozilla.org/r/63210/#review61420

> It doesn't make sense to refer internal header from exposed idl file.

Would you prefer if I copied the explaination from nsIWidget.h for this method?
I'm following what `void forceUpdateNativeMenuAt(in AString indexString);` is doing in the same file
I'm not sure what's a good convention for idl files.

> |this->| isn't necessary. And please check Destroyed() after this call.

nsChildView.h doesn't have a Destroyed() method. And this method is implemented on nsChildView so I don't think we need to check that the nsChildView widget exists.

widget/cocoa/TextInputHandler.h
has a Destroyed() method
```
nsChildView* mWidget; // [WEAK]
bool Destroyed() { return !mWidget; }
```

> I think that the name of this method and what this method does don't match.
> 
> This method returns selected text from eQuerySelectionAsTransferable event or nsClipboard::sSelectionTransferable (I don't know why we cannot use the latter every time, though).
> 
> (Note that when you don't need to dispatch WidgetQueryContentEvent, you shouldn't do that because it may be very expensive. You can check if focused content is in remote process with InputContext::mOrigin, that is set by SetInputContext() and it's always correct on focused widget.

nsClipboard::sSelectionTransferable is only set by nsAutoCopyListener in nsSelection.cpp for content selections. To get the chrome selection, I'm using the old DispatchWindowEvent which works on the chrome process.

> You can implement it as this in nsIWidget.h directly.

```
nsIWidget.h:1707:16: note: unimplemented pure virtual method 'TestTextSelectionOnServices' in 'PuppetWidget'
NS_IMETHOD TestTextSelectionOnServices(nsAString& aResult) = 0;
```
I get an error when I try to implement the method directly in nsIWidget.h

Comment 47

[Jimmy Wang (:jimicy) - works on e10s stuff]

https://reviewboard.mozilla.org/r/64144/#review61572

I have a flag set on WidgetQueryContentEvent in EventStateManager.cpp to determine what process it was dispatched in. However, I had a intermittent test where `GetFocusedContent()` was null despite a focused content.

**EventStateManager.cpp**
```
case eQueryContentState:
case eQuerySelectionAsTransferable:
  aEvent.isContentProcess = IsRemoteTarget(GetFocusedContent());
```

I printed out `fm->GetFocusedContent()` which existed, but `mDocument` was null. 
```
nsFocusManager* fm = nsFocusManager::GetFocusManager();
fm->GetFocusedContent()
```

I'm not using **isContentProcess** flag anymore, but I think this patch is still useful.

Comment 48

[Jimmy Wang (:jimicy) - works on e10s stuff]

Comment on attachment 8769248 [details]
Bug 1261299 - Add testing method GetSelectionAsPlaintext to DomWindowUtils which returns the text selection that was sent to the osx service menu nsChildView.mm.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63210/diff/4-5/

Comment 49

[Jimmy Wang (:jimicy) - works on e10s stuff]

Comment on attachment 8769249 [details]
Bug 1261299 - Add test for checking that the selected content/chrome text is properly sent up to the osx service menu code (nsChildView.mm).

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63212/diff/4-5/

Comment 50

[Jimmy Wang (:jimicy) - works on e10s stuff]

Comment on attachment 8769250 [details]
Bug 1261299 - On e10s in nsChildView for OSX services, determine if there is a current selection (chrome/content). Send the selection to osx service menu.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63214/diff/4-5/

Comment 51

[Jimmy Wang (:jimicy) - works on e10s stuff]

Comment on attachment 8770813 [details]
Bug 1261299 - Add EnsureDocument(mPresContext) to EventStateManager::GetFocusedContent() because mDocument is lazily loaded.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64144/diff/2-3/

Comment 52

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://reviewboard.mozilla.org/r/63210/#review61420

> Would you prefer if I copied the explaination from nsIWidget.h for this method?
> I'm following what `void forceUpdateNativeMenuAt(in AString indexString);` is doing in the same file
> I'm not sure what's a good convention for idl files.

It may need more discussion for reaching the best answer... However, nsIDOMWindowUtils.idl is used by some add-ons. So, for add-on developers, I think that we should make each method in every *.idl file clearer as far as possible. So, yes, I think that we should explain the method in idl rather than nsIWidget.h.

> nsChildView.h doesn't have a Destroyed() method. And this method is implemented on nsChildView so I don't think we need to check that the nsChildView widget exists.
> 
> widget/cocoa/TextInputHandler.h
> has a Destroyed() method
> ```
> nsChildView* mWidget; // [WEAK]
> bool Destroyed() { return !mWidget; }
> ```

nsChildView is a subclass of nsIWidget and nsIWidget has Destroyed().

Note that after dispatching an event, web application may close the window. Then, that may cause destroying the widget (destroying means a call of nsIWidget::Destroy(), not actually destroying the instance since it's kungFuDeathGripped by the caller). In such case, the widget should do nothing anymore.

> nsClipboard::sSelectionTransferable is only set by nsAutoCopyListener in nsSelection.cpp for content selections. To get the chrome selection, I'm using the old DispatchWindowEvent which works on the chrome process.

nsAutoCopyListener is works only with "autocopy" (select some text and middle click to paste it on Linux). So, it shouldn't work with macOS in default settings, no?

https://dxr.mozilla.org/mozilla-central/rev/4c05938a64a7fde3ac2d7f4493aee1c5f2ad8a0a/layout/generic/nsSelection.cpp#552-555
https://dxr.mozilla.org/mozilla-central/rev/4c05938a64a7fde3ac2d7f4493aee1c5f2ad8a0a/layout/generic/nsAutoCopyListener.h#25-28

And I'm surprised at that it's only available in content process??

Comment 53

[Jimmy Wang (:jimicy) - works on e10s stuff]

https://reviewboard.mozilla.org/r/63210/#review61420

> nsAutoCopyListener is works only with "autocopy" (select some text and middle click to paste it on Linux). So, it shouldn't work with macOS in default settings, no?
> 
> https://dxr.mozilla.org/mozilla-central/rev/4c05938a64a7fde3ac2d7f4493aee1c5f2ad8a0a/layout/generic/nsSelection.cpp#552-555
> https://dxr.mozilla.org/mozilla-central/rev/4c05938a64a7fde3ac2d7f4493aee1c5f2ad8a0a/layout/generic/nsAutoCopyListener.h#25-28
> 
> And I'm surprised at that it's only available in content process??

nsAutoCopyListener works in both parent and content process. I modified it so on macOS, it caches the content selection. The reason I did that only for content is because you can reselect a greyed out content selection by holding superkey+click and drag that selection. nsAutoCopyListener doesn't get retriggered in that case.
[https://www.dropbox.com/s/oj4q76blape0gjf/reselect-content-selection.gif?dl=0](https://www.dropbox.com/s/oj4q76blape0gjf/reselect-content-selection.gif?dl=0)


Thinking about it more. I think it makes more sense to keep 2 selection caches. This accounts for the being able to reselect a greyed out content selection.
**nsClipboard.h**
```
static mozilla::StaticRefPtr<nsITransferable> sContentSelectionTransferable; // content selection cache. current name: sSelectionTransferable
static mozilla::StaticRefPtr<nsITransferable> sChromeSelectionTransferable // #TODO add one for chrome selection cache 
```

Comment 54

[Jimmy Wang (:jimicy) - works on e10s stuff]

https://reviewboard.mozilla.org/r/63210/#review61420

> nsAutoCopyListener works in both parent and content process. I modified it so on macOS, it caches the content selection. The reason I did that only for content is because you can reselect a greyed out content selection by holding superkey+click and drag that selection. nsAutoCopyListener doesn't get retriggered in that case.
> [https://www.dropbox.com/s/oj4q76blape0gjf/reselect-content-selection.gif?dl=0](https://www.dropbox.com/s/oj4q76blape0gjf/reselect-content-selection.gif?dl=0)
> 
> 
> Thinking about it more. I think it makes more sense to keep 2 selection caches. This accounts for the being able to reselect a greyed out content selection.
> **nsClipboard.h**
> ```
> static mozilla::StaticRefPtr<nsITransferable> sContentSelectionTransferable; // content selection cache. current name: sSelectionTransferable
> static mozilla::StaticRefPtr<nsITransferable> sChromeSelectionTransferable // #TODO add one for chrome selection cache 
> ```

Looking into it more, nsClipboard::sSelectionTransferable can be used everything and set by nsAutoCopyListener::NotifySelectionChanged for chrome and content selection. My work around for the case above, can be resolved by triggering nsAutoCopyListener::NotifySelectionChanged when you click n drag to switch selection. I will post a new patch for review. Thanks!

Comment 55

[Markus Stange [:mstange]]

https://reviewboard.mozilla.org/r/63202/#review63752

I'd like to take another look at the fixed version of this patch.

::: layout/generic/nsAutoCopyListener.h:20
(Diff revision 3)
>  public:
>    NS_DECL_ISUPPORTS
>    NS_DECL_NSISELECTIONLISTENER
>  
> +  nsAutoCopyListener(int16_t aClipboardID)
> +  {

Please use the initializer list for this.


  nsAutoCopyListener(int16_t aClipboardID)
    : mCachedClipboard(aClipboardID)
  {}

http://en.cppreference.com/w/cpp/language/initializer_list

::: layout/generic/nsSelection.cpp:555
(Diff revision 3)
>    mSelectedCellIndex = 0;
>  
> +  nsAutoCopyListener *autoCopy = nullptr;
> +  // On E10s, cache the content selection for OSX service menu.
> +  if (XRE_IsContentProcess()) {
> +    #ifdef XP_MACOSX

Please wrap the #ifdef around the if, and don't indent the lines that start with a #.

::: widget/nsIClipboard.idl:21
(Diff revision 3)
>  {
>      const long kSelectionClipboard = 0;
>      const long kGlobalClipboard = 1;
>      const long kFindClipboard = 2;
> +    // Used for caching content selection on (nsClipboard) for OSX service menu.
> +    const long kSelectionTransferable = 3;

Let's rename this to kSelectionCache. All clipboards use nsITransferables internally, "cache" is more descriptive.

Comment 56

[Markus Stange [:mstange]]

Comment on attachment 8769245 [details]
Bug 1261299 - Add sSelectionTransferable and to get the current selection (chrome/content) needed for the OSX service menu.

https://reviewboard.mozilla.org/r/63204/#review63764

::: widget/cocoa/nsClipboard.h:30
(Diff revision 3)
>    NS_IMETHOD HasDataMatchingFlavors(const char** aFlavorList, uint32_t aLength,
>                                      int32_t aWhichClipboard, bool *_retval);
>    NS_IMETHOD SupportsFindClipboard(bool *_retval);
>  
> +  // Cache nsITransferable from content selection on OSX for service menu.
> +  static mozilla::StaticRefPtr<nsITransferable> sSelectionTransferable;

And let's rename this to sSelectionCache as well.

::: widget/cocoa/nsClipboard.mm:75
(Diff revision 3)
>  NS_IMETHODIMP
>  nsClipboard::SetNativeClipboardData(int32_t aWhichClipboard)
>  {
>    NS_OBJC_BEGIN_TRY_ABORT_BLOCK_NSRESULT;
>  
> +  // On E10s. Cache nsITransferable on chrome process from content selection on OSX for service menu.

// On macOS, cache the transferable of the content selection in the parent
  // process. This is needed for the services menu which requires synchronous
  // access to the current selection.

Comment 57

[Markus Stange [:mstange]]

Comment on attachment 8769246 [details]
Bug 1261299 - Add method nsCopySupport::ClearSelectionTransferable() to clear the nsClipboard::sSelectionTransferable when you have no content selection.

https://reviewboard.mozilla.org/r/63206/#review63772

SelectionTransferable -> SelectionCache everywhere here too.

::: dom/base/nsCopySupport.cpp:293
(Diff revision 3)
>    }
>    return SelectionCopyHelper(aSel, aDoc, true, aClipboardID, flags, nullptr);
>  }
>  
>  nsresult
> +nsCopySupport::ClearSelectionTransferable(){

Opening brace goes on the next line.

Comment 58

[Markus Stange [:mstange]]

Comment on attachment 8769244 [details]
Bug 1261299 - Add new clipboard kSelectionCache to cache the current selection for OSX service menu. Add a constructor to nsAutoCopyListener which sets the clipboard to copy to. Pass in kSelectionCache for OSX or kSelectionClipboard for linux.

https://reviewboard.mozilla.org/r/63202/#review63820

Comment 59

[Jimmy Wang (:jimicy) - works on e10s stuff]

Attachment: Bug 1261299 - Update the selection cache on repaint to handle when a pre-existing selection becomes active aka the current selection.

Review commit: https://reviewboard.mozilla.org/r/67480/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/67480/

Comment 60

[Jimmy Wang (:jimicy) - works on e10s stuff]

Comment on attachment 8769244 [details]
Bug 1261299 - Add new clipboard kSelectionCache to cache the current selection for OSX service menu. Add a constructor to nsAutoCopyListener which sets the clipboard to copy to. Pass in kSelectionCache for OSX or kSelectionClipboard for linux.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63202/diff/3-4/

Comment 61

[Jimmy Wang (:jimicy) - works on e10s stuff]

Comment on attachment 8769245 [details]
Bug 1261299 - Add sSelectionTransferable and to get the current selection (chrome/content) needed for the OSX service menu.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63204/diff/3-4/

Comment 62

[Jimmy Wang (:jimicy) - works on e10s stuff]

Comment on attachment 8769246 [details]
Bug 1261299 - Add method nsCopySupport::ClearSelectionTransferable() to clear the nsClipboard::sSelectionTransferable when you have no content selection.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63206/diff/3-4/

Comment 63

[Jimmy Wang (:jimicy) - works on e10s stuff]

Comment on attachment 8769248 [details]
Bug 1261299 - Add testing method GetSelectionAsPlaintext to DomWindowUtils which returns the text selection that was sent to the osx service menu nsChildView.mm.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63210/diff/5-6/

Comment 64

[Jimmy Wang (:jimicy) - works on e10s stuff]

Comment on attachment 8769249 [details]
Bug 1261299 - Add test for checking that the selected content/chrome text is properly sent up to the osx service menu code (nsChildView.mm).

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63212/diff/5-6/

Comment 65

[Jimmy Wang (:jimicy) - works on e10s stuff]

Comment on attachment 8769250 [details]
Bug 1261299 - On e10s in nsChildView for OSX services, determine if there is a current selection (chrome/content). Send the selection to osx service menu.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63214/diff/5-6/

Comment 66

[Jimmy Wang (:jimicy) - works on e10s stuff]

Comment on attachment 8770813 [details]
Bug 1261299 - Add EnsureDocument(mPresContext) to EventStateManager::GetFocusedContent() because mDocument is lazily loaded.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64144/diff/3-4/

Comment 67

[Markus Stange [:mstange]]

Comment on attachment 8769244 [details]
Bug 1261299 - Add new clipboard kSelectionCache to cache the current selection for OSX service menu. Add a constructor to nsAutoCopyListener which sets the clipboard to copy to. Pass in kSelectionCache for OSX or kSelectionClipboard for linux.

https://reviewboard.mozilla.org/r/63202/#review64640

Comment 68

[Jimmy Wang (:jimicy) - works on e10s stuff]

Attachment: Bug 1261299 - Only update nsClipboard::sSelectionCache in nsSelection.cpp when the selection listener is on the same process as the focused window where the selection occured or became active.

Review commit: https://reviewboard.mozilla.org/r/67754/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/67754/

Comment 69

[Jimmy Wang (:jimicy) - works on e10s stuff]

https://reviewboard.mozilla.org/r/67480/#review64808

::: layout/generic/nsSelection.cpp:1980
(Diff revision 1)
> +
> +// On macOS, update the cached selection to the active selection
> +// when you click and drag to make another selection active.
> +// Ex. Switching from a selection in a focusable input area to a selection on web page.
> +// See Bug 1288453.
> +#ifdef XP_MACOSX

Needed because the current selection cache doesn't get updated when you drag and click to make another selection active.

See Bug 1288453 for gif of the case when you need this.

Currently, **RepaintSelection** is called by both the chrome and content selection so NotifySelectionListeners is also called by both. It just so happens that the active selection's **RepaintSelection** is called at the end setting the current selection cache to the proper active selection.

However, what we should really be doing is checking that this is the current active selection. Only then do we call NotifySelectionListeners.

Comment 70

[Jimmy Wang (:jimicy) - works on e10s stuff]

Attachment: get-process-of-active-selection-not-working.gif

Comment 71

[Jimmy Wang (:jimicy) - works on e10s stuff]

https://reviewboard.mozilla.org/r/67480/#review64808

> Needed because the current selection cache doesn't get updated when you drag and click to make another selection active.
> 
> See Bug 1288453 for gif of the case when you need this.
> 
> Currently, **RepaintSelection** is called by both the chrome and content selection so NotifySelectionListeners is also called by both. It just so happens that the active selection's **RepaintSelection** is called at the end setting the current selection cache to the proper active selection.
> 
> However, what we should really be doing is checking that this is the current active selection. Only then do we call NotifySelectionListeners.

I'm having issues checking if the selection that is being repainted is the active selection.
See debug patch https://reviewboard.mozilla.org/r/67754/.

See gif https://bugzilla.mozilla.org/attachment.cgi?id=8775607
mIsContentProcess is always false despite the active selection being a content selection. You can test this by switching active selections
`printf("mIsContentProcess: %s \n",esm->IsRemoteTarget(focusedContent) ? "true" : "false");`

---

If you checked in
https://dxr.mozilla.org/mozilla-central/rev/db3ed1fdbbeaf5ab1e8fe454780146e7499be3db/dom/events/EventStateManager.cpp#859
**Content Selection**
IsRemoteTarget(GetFocusedContent()): true
**Chrome Selection**
IsRemoteTarget(GetFocusedContent()): false

Comment 72

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8769248 [details]
Bug 1261299 - Add testing method GetSelectionAsPlaintext to DomWindowUtils which returns the text selection that was sent to the osx service menu nsChildView.mm.

https://reviewboard.mozilla.org/r/63210/#review64984

::: dom/interfaces/base/nsIDOMWindowUtils.idl:788
(Diff revision 6)
> +   *
> +   * This is used for testing macOS service menu code.
> +   * This method should only be called if there is a current selection.
> +   * If there is no selection. NS_ERROR_FAILURE will be returned.
> +   */
> +  AString TestTextSelectionOnServices();

I still don't like this API name. Please rename it as what it does.

::: widget/cocoa/nsChildView.mm:5977
(Diff revision 6)
>    mGeckoChild->DispatchWindowEvent(command);
>  
>    return command.mSucceeded && command.mIsEnabled;
>  }
>  
> +nsresult nsChildView::TestTextSelectionOnServices(nsAString& aResult)

nit:

nsresult
nsChildView::Test...()
{

(break line after the type of result)

::: widget/cocoa/nsChildView.mm:5982
(Diff revision 6)
> +  NSDictionary* pasteboardOutputDict = nullptr;
> +
> +  if (nsClipboard::sSelectionCache) {
> +    // Get the current chrome or content selection.
> +    pasteboardOutputDict = nsClipboard::
> +        PasteboardDictFromTransferable(nsClipboard::sSelectionCache);
> +  }
> +
> +  if (!pasteboardOutputDict) {
> +    return NS_ERROR_FAILURE;
> +  }

Hmm, although, I'm not sure, I guess that if there are no selections, nsClipboard::sSelectionCache may be nullptr, isn't it?

If so, you should return NS_OK and make aResult empty since it's not unexpected case, isn't it?

So, I think this should be:

> if (!nsClipboard::sSelectionCache) {
>   MOZ_ASSERT(aResult.IsEmpty());
>   return NS_OK;
> }
> 
> // Get the current chrome or content selection.
> NSDictionary* pasteboardOutputDict =
>   nsClipboard::PasteboardDictFromTransferable(nsClipboard::sSelectionCache);
> if (NS_WARN_IF(!pasteboardOutputDict)) {
>   return NS_ERROR_FAILURE;
> }

::: widget/cocoa/nsChildView.mm:6002
(Diff revision 6)
> +  [declaredTypes addObjectsFromArray:[pasteboardOutputDict allKeys]];
> +  NSString* currentKey = [declaredTypes objectAtIndex:0];
> +  NSString* currentValue = [pasteboardOutputDict valueForKey:currentKey];
> +
> +  const char* textSelection = [currentValue UTF8String];
> +  AppendUTF8toUTF16(textSelection, aResult);

Why don't you use |aResult = NS_ConvertUTF8toUTF16(textSelection);|?

If aResult is not empty, this returns wrong value.

Or, just insert MOZ_ASSERT(aResult.IsEmpty()); next to NS_OBJECT_BEGIN_TRY_ABORT_BLOCK_NSRESULT;.

::: widget/nsIWidget.h:1730
(Diff revision 6)
> +     * If there is no selection. NS_ERROR_FAILURE will be returned.
> +     *
> +     * @param aResult - the current selected text.
> +     * @return nsresult - whether or not aResult was assigned the selected text.
> +     */
> +    virtual nsresult TestTextSelectionOnServices(nsAString& aResult)

And also this.

Comment 73

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8775335 [details]
Bug 1261299 - Update the selection cache on repaint to handle when a pre-existing selection becomes active aka the current selection.

https://reviewboard.mozilla.org/r/67480/#review64998

Hmm, I don't think I'm a good person for reviewing this patch. Why me? If you think that I should do it, please request the review again...

Comment 74

[Jimmy Wang (:jimicy) - works on e10s stuff]

https://reviewboard.mozilla.org/r/67480/#review64808

> I'm having issues checking if the selection that is being repainted is the active selection.
> See debug patch https://reviewboard.mozilla.org/r/67754/.
> 
> See gif https://bugzilla.mozilla.org/attachment.cgi?id=8775607
> mIsContentProcess is always false despite the active selection being a content selection. You can test this by switching active selections
> `printf("mIsContentProcess: %s \n",esm->IsRemoteTarget(focusedContent) ? "true" : "false");`
> 
> ---
> 
> If you checked in
> https://dxr.mozilla.org/mozilla-central/rev/db3ed1fdbbeaf5ab1e8fe454780146e7499be3db/dom/events/EventStateManager.cpp#859
> **Content Selection**
> IsRemoteTarget(GetFocusedContent()): true
> **Chrome Selection**
> IsRemoteTarget(GetFocusedContent()): false

Check if there exists a current focused window for nsSelection. Since there is an nsSelection for both chrome/content process.
This way we only call NotifySelectionListeners if nsSelection is on the same process as the current focused window.
```
  nsFocusManager* fm = nsFocusManager::GetFocusManager();

  if (fm->GetActiveWindow()) {
    PostReason(nsISelectionListener::MOUSEUP_REASON);
    NotifySelectionListeners(SelectionType::eNormal);
  }
```

Comment 75

[Jimmy Wang (:jimicy) - works on e10s stuff]

Comment on attachment 8769244 [details]
Bug 1261299 - Add new clipboard kSelectionCache to cache the current selection for OSX service menu. Add a constructor to nsAutoCopyListener which sets the clipboard to copy to. Pass in kSelectionCache for OSX or kSelectionClipboard for linux.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63202/diff/4-5/

Comment 76

[Jimmy Wang (:jimicy) - works on e10s stuff]

Comment on attachment 8769245 [details]
Bug 1261299 - Add sSelectionTransferable and to get the current selection (chrome/content) needed for the OSX service menu.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63204/diff/4-5/

Comment 77

[Jimmy Wang (:jimicy) - works on e10s stuff]

Comment on attachment 8769246 [details]
Bug 1261299 - Add method nsCopySupport::ClearSelectionTransferable() to clear the nsClipboard::sSelectionTransferable when you have no content selection.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63206/diff/4-5/

Comment 78

[Jimmy Wang (:jimicy) - works on e10s stuff]

Comment on attachment 8769250 [details]
Bug 1261299 - On e10s in nsChildView for OSX services, determine if there is a current selection (chrome/content). Send the selection to osx service menu.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63214/diff/6-7/

Comment 79

[Jimmy Wang (:jimicy) - works on e10s stuff]

Comment on attachment 8770813 [details]
Bug 1261299 - Add EnsureDocument(mPresContext) to EventStateManager::GetFocusedContent() because mDocument is lazily loaded.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64144/diff/4-5/

Comment 80

[Jimmy Wang (:jimicy) - works on e10s stuff]

Comment on attachment 8775335 [details]
Bug 1261299 - Update the selection cache on repaint to handle when a pre-existing selection becomes active aka the current selection.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67480/diff/1-2/

Comment 81

[Jimmy Wang (:jimicy) - works on e10s stuff]

Comment on attachment 8775590 [details]
Bug 1261299 - Only update nsClipboard::sSelectionCache in nsSelection.cpp when the selection listener is on the same process as the focused window where the selection occured or became active.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67754/diff/1-2/

Comment 82

[Jimmy Wang (:jimicy) - works on e10s stuff]

Comment on attachment 8769248 [details]
Bug 1261299 - Add testing method GetSelectionAsPlaintext to DomWindowUtils which returns the text selection that was sent to the osx service menu nsChildView.mm.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63210/diff/6-7/

Comment 83

[Jimmy Wang (:jimicy) - works on e10s stuff]

Comment on attachment 8769249 [details]
Bug 1261299 - Add test for checking that the selected content/chrome text is properly sent up to the osx service menu code (nsChildView.mm).

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63212/diff/6-7/

Comment 84

[Jimmy Wang (:jimicy) - works on e10s stuff]

https://reviewboard.mozilla.org/r/67754/#review66220

nsAutoCopyListener::NotifySelectionChanged called in the wrong process

::: layout/generic/nsSelection.cpp:6497
(Diff revision 2)
>  
>  NS_IMETHODIMP
>  nsAutoCopyListener::NotifySelectionChanged(nsIDOMDocument *aDoc,
>                                             nsISelection *aSel, int16_t aReason)
>  {
> +  if (mCachedClipboard == nsIClipboard::kSelectionCache) {

Needed because `nsAutoCopyListener::NotifySelectionChanged` gets called to update the selection where
`nsCopySupport::HTMLCopy | XRE_IsContentProcess(): true | fm->GetActiveWindow(): false`
happens for **test_content_and_chrome_selection()** in browser_bug1261299.js

What this means is that the current selection cache is set to the content process selection overriding the active chrome selection that was cached. It doesn't make sense for `NotifySelectionChanged` to be called if you're not in the same window where you're focused in and made the selection.

Stack trace of who calls `nsAutoCopyListener::NotifySelectionChanged` in the wrong process
```
(lldb) p fm->GetActiveWindow()
(nsPIDOMWindowOuter *) $1 = 0x0000000000000000
(lldb) bt
* thread #1: tid = 0x1b42cc, 0x000000010c108fa9 XUL`nsAutoCopyListener::NotifySelectionChanged(this=0x0000000129b07160, aDoc=0x0000000120d5b000, aSel=0x0000000122039d70, aReason=8) + 521 at nsSelection.cpp:6534, queue = 'com.apple.main-thread', stop reason = breakpoint 1.1
  * frame #0: 0x000000010c108fa9 XUL`nsAutoCopyListener::NotifySelectionChanged(this=0x0000000129b07160, aDoc=0x0000000120d5b000, aSel=0x0000000122039d70, aReason=8) + 521 at nsSelection.cpp:6534
    frame #1: 0x000000010c0fc75f XUL`mozilla::dom::Selection::NotifySelectionListeners(this=0x0000000122039d70) + 399 at nsSelection.cpp:6179
    frame #2: 0x000000010c0fa8d3 XUL`nsFrameSelection::NotifySelectionListeners(this=0x0000000120d5d4f0, aSelectionType=eNormal) + 131 at nsSelection.cpp:2421
    frame #3: 0x000000010c1069d9 XUL`mozilla::dom::Selection::Extend(this=0x0000000122039d70, aParentNode="Write something here", aOffset=20, aRv=0x00007fff5919bd60) + 5017 at nsSelection.cpp:5729
    frame #4: 0x000000010c0f98cf XUL`mozilla::dom::Selection::Extend(this=0x0000000122039d70, aParentNode="Write something here", aOffset=20) + 79 at nsSelection.cpp:5441
    frame #5: 0x000000010c0f7444 XUL`nsFrameSelection::TakeFocus(this=0x0000000120d5d4f0, aNewFocus="Write something here", aContentOffset=20, aContentEndOffset=20, aHint=CARET_ASSOCIATE_BEFORE, aContinueSelection=true, aMultipleSelection=false) + 2100 at nsSelection.cpp:1868
    frame #6: 0x000000010c0f5bf2 XUL`nsFrameSelection::MoveCaret(this=0x0000000120d5d4f0, aDirection=eDirNext, aContinueSelection=true, aAmount=eSelectEndLine, aMovementStyle=eLogical) + 3122 at nsSelection.cpp:1155
    frame #7: 0x000000010c0fc1ec XUL`nsFrameSelection::IntraLineMove(this=0x0000000120d5d4f0, aForward=true, aExtend=true) + 76 at nsSelection.cpp:2358
```

I am not able to produce this case manually. But my test requires this check.
I made this behavior specific for OSX, but maybe the linux autocopy should also be doing this same check?

Comment 85

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8769248 [details]
Bug 1261299 - Add testing method GetSelectionAsPlaintext to DomWindowUtils which returns the text selection that was sent to the osx service menu nsChildView.mm.

https://reviewboard.mozilla.org/r/63210/#review66242

::: dom/interfaces/base/nsIDOMWindowUtils.idl:786
(Diff revision 7)
> +   * See nsIWidget::GetCurrentTextSelectionSentToServices
> +   *
> +   * This is used for testing macOS service menu code.
> +   * This method should only be called if there is a current selection.
> +   * If there is no selection. NS_ERROR_FAILURE will be returned.
> +   */
> +  AString GetCurrentTextSelectionSentToServices();

Hmm, I still don't like "SentDoServices" because it's difficult to understand what the "Service" means and it doesn't make sense for the other platforms.

How about GetSelectionAsPlaintext()? Explain as:

/**
 * This returns selection as plaintext.  Note that the result may be
 * different from the result of sendQueryContentEvent(QUERY_SELECTED_TEXT).
 * This result is computed by native API with trasfarable data.  In other
 * words, When OS treats the selection as plaintext, it treats current
 * selection as this result.
 */

?

Comment 86

[Jimmy Wang (:jimicy) - works on e10s stuff]

Comment on attachment 8769248 [details]
Bug 1261299 - Add testing method GetSelectionAsPlaintext to DomWindowUtils which returns the text selection that was sent to the osx service menu nsChildView.mm.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63210/diff/7-8/

Comment 87

[Jimmy Wang (:jimicy) - works on e10s stuff]

Comment on attachment 8769249 [details]
Bug 1261299 - Add test for checking that the selected content/chrome text is properly sent up to the osx service menu code (nsChildView.mm).

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63212/diff/7-8/

Comment 88

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8769248 [details]
Bug 1261299 - Add testing method GetSelectionAsPlaintext to DomWindowUtils which returns the text selection that was sent to the osx service menu nsChildView.mm.

https://reviewboard.mozilla.org/r/63210/#review66270

Thanks!

Comment 89

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8775335 [details]
Bug 1261299 - Update the selection cache on repaint to handle when a pre-existing selection becomes active aka the current selection.

https://reviewboard.mozilla.org/r/67480/#review66350

I think I'm missing something here. I don't understand why repainting would be mouseup.

::: layout/generic/nsSelection.cpp:1981
(Diff revision 2)
> +// On macOS, update the cached selection to the active selection
> +// when you click and drag to make another selection active.
> +// Ex. Switching from a selection in a focusable input area to a selection on web page.
> +// See Bug 1288453.
> +#ifdef XP_MACOSX
> +  PostReason(nsISelectionListener::MOUSEUP_REASON);

Why MOUSEUP_REASON?

::: layout/generic/nsSelection.cpp:1982
(Diff revision 2)
> +// when you click and drag to make another selection active.
> +// Ex. Switching from a selection in a focusable input area to a selection on web page.
> +// See Bug 1288453.
> +#ifdef XP_MACOSX
> +  PostReason(nsISelectionListener::MOUSEUP_REASON);
> +  NotifySelectionListeners(SelectionType::eNormal);

I don't understand this. aSelectionType can be anything, but we call NotifySelectionListeners with eNormal.

Comment 90

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8775590 [details]
Bug 1261299 - Only update nsClipboard::sSelectionCache in nsSelection.cpp when the selection listener is on the same process as the focused window where the selection occured or became active.

https://reviewboard.mozilla.org/r/67754/#review66356

This is on top of the previous patch which I don't quite understand.

Comment 91

[Jimmy Wang (:jimicy) - works on e10s stuff]

Attachment: eFind-highlight-all-selection-and-eNormal-selection

Comment 92

[Olli Pettay [:smaug][[e-mail]]]

> ::: layout/generic/nsSelection.cpp:1981
> (Diff revision 2)
> > +// On macOS, update the cached selection to the active selection
> > +// when you click and drag to make another selection active.
> > +// Ex. Switching from a selection in a focusable input area to a selection on web page.
> > +// See Bug 1288453.
> > +#ifdef XP_MACOSX
> > +  PostReason(nsISelectionListener::MOUSEUP_REASON);
> 
> Why MOUSEUP_REASON?

Do we need a new reason for this? Or could we use NOREASON?

Comment 93

[Jimmy Wang (:jimicy) - works on e10s stuff]

https://reviewboard.mozilla.org/r/67480/#review66350

> Why MOUSEUP_REASON?

For **nsAutoCopyListener::NotifySelectionChanged**. Because I need to update the current selection cache to the current active selection when they get repainted.
I needed to pass in a reason and MOUSEUP_REASON was the closest to the case where you click and drag a selection to change which is active.
I can make a new reason `nsISelectionListener::REPAINT_SELECTION` if that is more clear.

```
if (!(aReason & nsISelectionListener::MOUSEUP_REASON   || 
        aReason & nsISelectionListener::SELECTALL_REASON ||
        aReason & nsISelectionListener::KEYPRESS_REASON))
    return NS_OK; //dont care if we are still dragging
```

> I don't understand this. aSelectionType can be anything, but we call NotifySelectionListeners with eNormal.

I'm only interested in updating the current selection cache for eNormal selections.
Only **eNormal** selections get called to RepaintSelection since they can become inactive (greyed out), other selections do not get passed to repaint.

I can be more explicit and say if `aSelectionType == SelectionType::eNormal` then NotifySelectionListeners.

Pic displaying eFind and eNormal selections: https://bugzilla.mozilla.org/attachment.cgi?id=8777819

Comment 94

[Olli Pettay [:smaug][[e-mail]]]

ahaa, ok, so clearly the code needs some comments why special casing normal. And MOUSEUP feels still rather hacky.

Could the cache be updated at some other time, like when window is activated/deactivated? Why this is bound to the repainting?

Comment 95

[Jimmy Wang (:jimicy) - works on e10s stuff]

Comment on attachment 8769244 [details]
Bug 1261299 - Add new clipboard kSelectionCache to cache the current selection for OSX service menu. Add a constructor to nsAutoCopyListener which sets the clipboard to copy to. Pass in kSelectionCache for OSX or kSelectionClipboard for linux.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63202/diff/5-6/

Comment 96

[Jimmy Wang (:jimicy) - works on e10s stuff]

Comment on attachment 8769245 [details]
Bug 1261299 - Add sSelectionTransferable and to get the current selection (chrome/content) needed for the OSX service menu.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63204/diff/5-6/

Comment 97

[Jimmy Wang (:jimicy) - works on e10s stuff]

Comment on attachment 8769246 [details]
Bug 1261299 - Add method nsCopySupport::ClearSelectionTransferable() to clear the nsClipboard::sSelectionTransferable when you have no content selection.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63206/diff/5-6/

Comment 98

[Jimmy Wang (:jimicy) - works on e10s stuff]

Comment on attachment 8769250 [details]
Bug 1261299 - On e10s in nsChildView for OSX services, determine if there is a current selection (chrome/content). Send the selection to osx service menu.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63214/diff/7-8/

Comment 99

[Jimmy Wang (:jimicy) - works on e10s stuff]

Comment on attachment 8770813 [details]
Bug 1261299 - Add EnsureDocument(mPresContext) to EventStateManager::GetFocusedContent() because mDocument is lazily loaded.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64144/diff/5-6/

Comment 100

[Jimmy Wang (:jimicy) - works on e10s stuff]

Comment on attachment 8775335 [details]
Bug 1261299 - Update the selection cache on repaint to handle when a pre-existing selection becomes active aka the current selection.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67480/diff/2-3/

Comment 101

[Jimmy Wang (:jimicy) - works on e10s stuff]

Comment on attachment 8775590 [details]
Bug 1261299 - Only update nsClipboard::sSelectionCache in nsSelection.cpp when the selection listener is on the same process as the focused window where the selection occured or became active.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67754/diff/2-3/

Comment 102

[Jimmy Wang (:jimicy) - works on e10s stuff]

Comment on attachment 8769248 [details]
Bug 1261299 - Add testing method GetSelectionAsPlaintext to DomWindowUtils which returns the text selection that was sent to the osx service menu nsChildView.mm.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63210/diff/8-9/

Comment 103

[Jimmy Wang (:jimicy) - works on e10s stuff]

Comment on attachment 8769249 [details]
Bug 1261299 - Add test for checking that the selected content/chrome text is properly sent up to the osx service menu code (nsChildView.mm).

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63212/diff/8-9/

Comment 104

[Jimmy Wang (:jimicy) - works on e10s stuff]

Comment on attachment 8775335 [details]
Bug 1261299 - Update the selection cache on repaint to handle when a pre-existing selection becomes active aka the current selection.

https://reviewboard.mozilla.org/r/67480/#review67674

::: layout/generic/nsSelection.cpp:1978
(Diff revision 3)
>      return NS_ERROR_NULL_POINTER;
>    NS_ENSURE_STATE(mShell);
> +
> +// On macOS, update the cached selection to the active selection
> +#ifdef XP_MACOSX
> +  if (aSelectionType == SelectionType::eNormal) {

To account for when a pre-exisiting selection becomes active aka the current selection.

Instead of `PostReason(nsISelectionListener::MOUSE_UP);` call an new method `UpdateSelectionCacheOnRepaintSelection` which handles setting the selection cache on a repaint.

We want to use RepaintSelection as it is always called when the selection state changes.

`NotifyFocusStateChange` in nsFocusManager doesn't handle when you click n drag a selection to change the current selection.

https://developer.mozilla.org/en/docs/Web/HTML/Element/iframe
1. Make a selection on paragraph text
2. Make a text selection in the iframe
3. Click n drag to switch the active selection

Comment 105

[Jimmy Wang (:jimicy) - works on e10s stuff]

Attachment: Bug 1261299 - Add a method nsCopySupport::SetSelectionCache instead of relying on the method used to set the os clipboard.

Review commit: https://reviewboard.mozilla.org/r/70420/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/70420/

Comment 106

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8775335 [details]
Bug 1261299 - Update the selection cache on repaint to handle when a pre-existing selection becomes active aka the current selection.

https://reviewboard.mozilla.org/r/67480/#review67726

I'll get back to this patch...

::: layout/generic/nsSelection.cpp:6537
(Diff revision 3)
> + *
> + * If the current selection is empty. The current selection cache
> + * would be cleared by nsAutoCopyListener::NotifySelectionChanged.
> + */
> +nsresult
> +nsFrameSelection::UpdateSelectionCacheOnRepaintSelection(Selection *aSel)

Nit, Selection* aSel

::: layout/generic/nsSelection.cpp:6550
(Diff revision 3)
> +  bool collapsed;
> +  if (aDoc && aSel &&
> +      NS_SUCCEEDED(aSel->GetIsCollapsed(&collapsed)) && !collapsed) {
> +    nsCOMPtr<nsIDocument> doc = do_QueryInterface(aDoc);
> +    NS_ENSURE_TRUE(doc, NS_ERROR_FAILURE);
> +    return nsCopySupport::HTMLCopy(aSel, doc,

I don't yet understand this, need to check what adds kSelectionCache

Comment 107

[Jimmy Wang (:jimicy) - works on e10s stuff]

Comment on attachment 8775335 [details]
Bug 1261299 - Update the selection cache on repaint to handle when a pre-existing selection becomes active aka the current selection.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67480/diff/3-4/

Comment 108

[Jimmy Wang (:jimicy) - works on e10s stuff]

Comment on attachment 8775590 [details]
Bug 1261299 - Only update nsClipboard::sSelectionCache in nsSelection.cpp when the selection listener is on the same process as the focused window where the selection occured or became active.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67754/diff/3-4/

Comment 109

[Jimmy Wang (:jimicy) - works on e10s stuff]

Comment on attachment 8769248 [details]
Bug 1261299 - Add testing method GetSelectionAsPlaintext to DomWindowUtils which returns the text selection that was sent to the osx service menu nsChildView.mm.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63210/diff/9-10/

Comment 110

[Jimmy Wang (:jimicy) - works on e10s stuff]

Comment on attachment 8769249 [details]
Bug 1261299 - Add test for checking that the selected content/chrome text is properly sent up to the osx service menu code (nsChildView.mm).

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63212/diff/9-10/

Comment 111

[Jimmy Wang (:jimicy) - works on e10s stuff]

Comment on attachment 8779420 [details]
Bug 1261299 - Add a method nsCopySupport::SetSelectionCache instead of relying on the method used to set the os clipboard.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/70420/diff/1-2/

Comment 112

[Jimmy Wang (:jimicy) - works on e10s stuff]

Comment on attachment 8779420 [details]
Bug 1261299 - Add a method nsCopySupport::SetSelectionCache instead of relying on the method used to set the os clipboard.

https://reviewboard.mozilla.org/r/70420/#review67756

::: dom/base/nsCopySupport.cpp:411
(Diff revision 2)
> +    rv = docEncoder->EncodeToStringWithContext(htmlParentsBuf, htmlInfoBuf,
> +                                               textHTMLBuf);
> +    NS_ENSURE_SUCCESS(rv, rv);
> +  }
> +
> +  if (aTransferable != nullptr) {

Test were not failing before
https://treeherder.mozilla.org/#/jobs?repo=try&revision=72d3eb8fd7a2
Now they are failing for the same method call to **SelectionCopyHelper**.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d6da3553ede4

I pulled and rebased on central the try push for when they were passing and they no longer pass.

Failing test
browser/base/content/test/general/browser_clipboard_pastefile.js

```
Browser Chrome Test Summary
  Passed: 4
  Failed: 2
  Todo: 0
  Mode: e10s
*** End BrowserChrome Test Results ***
The following tests failed:
21 INFO TEST-UNEXPECTED-FAIL | browser/base/content/test/general/browser_clipboard_pastefile.js | Paste file - Got Wrong number of types; got 1, expected Passed
Stack trace:
    chrome://mochikit/content/browser-test.js:test_is:967
    chrome://mochitests/content/browser/browser/base/content/test/general/browser_clipboard_pastefile.js:null:37
    Tester_execTest@chrome://mochikit/content/browser-test.js:791:9
    Tester.prototype.nextTest</<@chrome://mochikit/content/browser-test.js:711:7
    SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:741:59
22 INFO TEST-UNEXPECTED-FAIL | browser/base/content/test/general/browser_clipboard_pastefile.js | number of types - Got 1, expected 3
Stack trace:
    chrome://mochikit/content/browser-test.js:test_is:967
    chrome://mochitests/content/browser/browser/base/content/test/general/browser_clipboard_pastefile.js:copyEvent:46
    chrome://global/content/bindings/autocomplete.xml:.doCommand:587
    chrome://mochikit/content/tests/SimpleTest/EventUtils.js:synthesizeKey:774
    chrome://mochitests/content/browser/browser/base/content/test/general/browser_clipboard_pastefile.js:null:55
    resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:Promise:388
    chrome://mochitests/content/browser/browser/base/content/test/general/browser_clipboard_pastefile.js:null:41
    Tester_execTest@chrome://mochikit/content/browser-test.js:791:9
    Tester.prototype.nextTest</<@chrome://mochikit/content/browser-test.js:711:7
    SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:741:59
```

```
console.log: DOMStringList {"0":"text/plain"}
```

Suppose to be
```
console.log: DOMStringList {"0":"application/x-moz-file","1":"text/plain","2":"Files"}
```

I don't see how creating a Transferable would affect `event.clipboardData` in browser_clipboard_pastefile.js

In **SelectionCopyHelper2**, I removed the clipboard if blocks that don't get triggered and narrowed down this code block as the culprit.
```
if (aTransferable != nullptr) { ... }
```

Comment 113

[Jimmy Wang (:jimicy) - works on e10s stuff]

Comment on attachment 8779420 [details]
Bug 1261299 - Add a method nsCopySupport::SetSelectionCache instead of relying on the method used to set the os clipboard.

https://reviewboard.mozilla.org/r/70420/#review67756

> Test were not failing before
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=72d3eb8fd7a2
> Now they are failing for the same method call to **SelectionCopyHelper**.
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=d6da3553ede4
> 
> I pulled and rebased on central the try push for when they were passing and they no longer pass.
> 
> Failing test
> browser/base/content/test/general/browser_clipboard_pastefile.js
> 
> ```
> Browser Chrome Test Summary
>   Passed: 4
>   Failed: 2
>   Todo: 0
>   Mode: e10s
> *** End BrowserChrome Test Results ***
> The following tests failed:
> 21 INFO TEST-UNEXPECTED-FAIL | browser/base/content/test/general/browser_clipboard_pastefile.js | Paste file - Got Wrong number of types; got 1, expected Passed
> Stack trace:
>     chrome://mochikit/content/browser-test.js:test_is:967
>     chrome://mochitests/content/browser/browser/base/content/test/general/browser_clipboard_pastefile.js:null:37
>     Tester_execTest@chrome://mochikit/content/browser-test.js:791:9
>     Tester.prototype.nextTest</<@chrome://mochikit/content/browser-test.js:711:7
>     SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:741:59
> 22 INFO TEST-UNEXPECTED-FAIL | browser/base/content/test/general/browser_clipboard_pastefile.js | number of types - Got 1, expected 3
> Stack trace:
>     chrome://mochikit/content/browser-test.js:test_is:967
>     chrome://mochitests/content/browser/browser/base/content/test/general/browser_clipboard_pastefile.js:copyEvent:46
>     chrome://global/content/bindings/autocomplete.xml:.doCommand:587
>     chrome://mochikit/content/tests/SimpleTest/EventUtils.js:synthesizeKey:774
>     chrome://mochitests/content/browser/browser/base/content/test/general/browser_clipboard_pastefile.js:null:55
>     resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:Promise:388
>     chrome://mochitests/content/browser/browser/base/content/test/general/browser_clipboard_pastefile.js:null:41
>     Tester_execTest@chrome://mochikit/content/browser-test.js:791:9
>     Tester.prototype.nextTest</<@chrome://mochikit/content/browser-test.js:711:7
>     SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:741:59
> ```
> 
> ```
> console.log: DOMStringList {"0":"text/plain"}
> ```
> 
> Suppose to be
> ```
> console.log: DOMStringList {"0":"application/x-moz-file","1":"text/plain","2":"Files"}
> ```
> 
> I don't see how creating a Transferable would affect `event.clipboardData` in browser_clipboard_pastefile.js
> 
> In **SelectionCopyHelper2**, I removed the clipboard if blocks that don't get triggered and narrowed down this code block as the culprit.
> ```
> if (aTransferable != nullptr) { ... }
> ```

**NotifySelectionChanged** ends up calling **SelectionCopyHelper** and these extra calls to **SelectionCopyHelper** were causing the test to fail.
**SelectionCopyHelper2** was made to separate out exactly what was causing the error. I call **SelectionCopyHelper2** with the same args as **nsCopySupport::GetTransferableForSelection**. This avoids any code that triggers to set data on the native os clipboard since the test fail was `event.clipboardData.types` having the wrong number of types, 1 instead of 3.

Comment 114

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8775335 [details]
Bug 1261299 - Update the selection cache on repaint to handle when a pre-existing selection becomes active aka the current selection.

https://reviewboard.mozilla.org/r/67480/#review67784

ok, after looking some other patches, I think I'm starting to understand the setup :)

::: layout/generic/nsSelection.cpp:6538
(Diff revision 4)
> + * If the current selection is empty. The current selection cache
> + * would be cleared by nsAutoCopyListener::NotifySelectionChanged.
> + */
> +nsresult
> +nsFrameSelection::UpdateSelectionCacheOnRepaintSelection(Selection* aSel)
> +{

only arguments should be in form aFoo, so rename aDoc to doc

::: layout/generic/nsSelection.cpp:6541
(Diff revision 4)
> +nsresult
> +nsFrameSelection::UpdateSelectionCacheOnRepaintSelection(Selection* aSel)
> +{
> +  nsCOMPtr<nsIDOMDocument> aDoc;
> +  nsIPresShell* ps = aSel->GetPresShell();
> +  if (ps) {

might be nicer to write

if (!ps) {
  return NS_OK;
}

doc = do_QueryInterface(ps->GetDocument());

Comment 115

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8775590 [details]
Bug 1261299 - Only update nsClipboard::sSelectionCache in nsSelection.cpp when the selection listener is on the same process as the focused window where the selection occured or became active.

https://reviewboard.mozilla.org/r/67754/#review67812

So I think this needs some updates to comments.

::: layout/generic/nsSelection.cpp:1982
(Diff revision 4)
>  // On macOS, update the cached selection to the active selection
>  #ifdef XP_MACOSX
> -  if (aSelectionType == SelectionType::eNormal) {
> +  nsFocusManager* fm = nsFocusManager::GetFocusManager();
> +  // Check that nsFrameSelection is on the same process as the focused window
> +  // where the current active selection is on and that it's a normal selection.
> +  if (fm->GetActiveWindow() && aSelectionType == SelectionType::eNormal) {

I don't understand how this does any same-process check. Please explain what this is trying to do.

::: layout/generic/nsSelection.cpp:6497
(Diff revision 4)
>  {
> +  if (mCachedClipboard == nsIClipboard::kSelectionCache) {
> +    nsFocusManager* fm = nsFocusManager::GetFocusManager();
> +    // If the nsAutoCopyListener is not on the same process as the focused
> +    // window where the selection occured then don't update the selection cache.
> +    if (!fm->GetActiveWindow()) {

Same here.

GetActiveWindow doesn't really have anything to do with processes.
Doesn't it return null for example when all the FF windows are minimized.

Comment 116

[Jimmy Wang (:jimicy) - works on e10s stuff]

Comment on attachment 8775590 [details]
Bug 1261299 - Only update nsClipboard::sSelectionCache in nsSelection.cpp when the selection listener is on the same process as the focused window where the selection occured or became active.

https://reviewboard.mozilla.org/r/67754/#review67812

> I don't understand how this does any same-process check. Please explain what this is trying to do.

nsSelection is on both chrome and content process. nsFrameSelection::RepaintSelection would get called for both. Without a check in place. You could potentially have the cache for a current selection in the chrome process overridden by no selection in the content process.

Updated comment. Thoughts?
```
// Check an active window exists otherwise there cannot be a current selection
// and that it's a normal selection.
```

> Same here.
> 
> GetActiveWindow doesn't really have anything to do with processes.
> Doesn't it return null for example when all the FF windows are minimized.

Updated comment. Thoughts?
```
// If no active window, do nothing because a current selection changed
// cannot occur unless it is in the active window.
```

I couldn't get this to occur manually. But my test browser_bug1261299.js manages to make this happen.
You shouldn't be able to have a current selection (active selection) unless it is in your active window.

So far I'm making this macOS specific. But maybe this should also be added for autocopy for linux since the same idea holds?

Comment 118

[Jimmy Wang (:jimicy) - works on e10s stuff]

Comment on attachment 8769244 [details]
Bug 1261299 - Add new clipboard kSelectionCache to cache the current selection for OSX service menu. Add a constructor to nsAutoCopyListener which sets the clipboard to copy to. Pass in kSelectionCache for OSX or kSelectionClipboard for linux.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63202/diff/6-7/

Comment 119

[Jimmy Wang (:jimicy) - works on e10s stuff]

Comment on attachment 8769245 [details]
Bug 1261299 - Add sSelectionTransferable and to get the current selection (chrome/content) needed for the OSX service menu.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63204/diff/6-7/

Comment 120

[Jimmy Wang (:jimicy) - works on e10s stuff]

Comment on attachment 8769246 [details]
Bug 1261299 - Add method nsCopySupport::ClearSelectionTransferable() to clear the nsClipboard::sSelectionTransferable when you have no content selection.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63206/diff/6-7/

Comment 121

[Jimmy Wang (:jimicy) - works on e10s stuff]

Comment on attachment 8769250 [details]
Bug 1261299 - On e10s in nsChildView for OSX services, determine if there is a current selection (chrome/content). Send the selection to osx service menu.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63214/diff/8-9/

Comment 122

[Jimmy Wang (:jimicy) - works on e10s stuff]

Comment on attachment 8770813 [details]
Bug 1261299 - Add EnsureDocument(mPresContext) to EventStateManager::GetFocusedContent() because mDocument is lazily loaded.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64144/diff/6-7/

Comment 123

[Jimmy Wang (:jimicy) - works on e10s stuff]

Comment on attachment 8775335 [details]
Bug 1261299 - Update the selection cache on repaint to handle when a pre-existing selection becomes active aka the current selection.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67480/diff/4-5/

Comment 124

[Jimmy Wang (:jimicy) - works on e10s stuff]

Comment on attachment 8775590 [details]
Bug 1261299 - Only update nsClipboard::sSelectionCache in nsSelection.cpp when the selection listener is on the same process as the focused window where the selection occured or became active.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67754/diff/4-5/

Comment 125

[Jimmy Wang (:jimicy) - works on e10s stuff]

Comment on attachment 8769248 [details]
Bug 1261299 - Add testing method GetSelectionAsPlaintext to DomWindowUtils which returns the text selection that was sent to the osx service menu nsChildView.mm.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63210/diff/10-11/

Comment 126

[Jimmy Wang (:jimicy) - works on e10s stuff]

Comment on attachment 8769249 [details]
Bug 1261299 - Add test for checking that the selected content/chrome text is properly sent up to the osx service menu code (nsChildView.mm).

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63212/diff/10-11/

Comment 127

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8775590 [details]
Bug 1261299 - Only update nsClipboard::sSelectionCache in nsSelection.cpp when the selection listener is on the same process as the focused window where the selection occured or became active.

https://reviewboard.mozilla.org/r/67754/#review68392

Comment 128

[Jimmy Wang (:jimicy) - works on e10s stuff]

Attachment: Bug 1261299 - Implement nsIClipboard methods in nsClipboard.mm with methods specific for osx. (EmptyClipboard, SetData) methods modified from nsBaseClipboard to also set the current selection cache.

Review commit: https://reviewboard.mozilla.org/r/70972/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/70972/

Comment 129

[Jimmy Wang (:jimicy) - works on e10s stuff]

Comment on attachment 8780275 [details]
Bug 1261299 - Implement nsIClipboard methods in nsClipboard.mm with methods specific for osx. (EmptyClipboard, SetData) methods modified from nsBaseClipboard to also set the current selection cache.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/70972/diff/1-2/

Comment 130

[Jimmy Wang (:jimicy) - works on e10s stuff]

Comment on attachment 8780275 [details]
Bug 1261299 - Implement nsIClipboard methods in nsClipboard.mm with methods specific for osx. (EmptyClipboard, SetData) methods modified from nsBaseClipboard to also set the current selection cache.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/70972/diff/2-3/

Comment 131

[Jimmy Wang (:jimicy) - works on e10s stuff]

Comment on attachment 8780275 [details]
Bug 1261299 - Implement nsIClipboard methods in nsClipboard.mm with methods specific for osx. (EmptyClipboard, SetData) methods modified from nsBaseClipboard to also set the current selection cache.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/70972/diff/3-4/

Comment 132

[Jimmy Wang (:jimicy) - works on e10s stuff]

Comment on attachment 8780275 [details]
Bug 1261299 - Implement nsIClipboard methods in nsClipboard.mm with methods specific for osx. (EmptyClipboard, SetData) methods modified from nsBaseClipboard to also set the current selection cache.

https://reviewboard.mozilla.org/r/70972/#review68510

::: widget/cocoa/nsClipboard.mm:687
(Diff revision 4)
> +/**
> +  * Sets the transferable object
> +  *
> +  */
> +NS_IMETHODIMP
> +nsClipboard::SetData(nsITransferable* aTransferable, nsIClipboardOwner* anOwner,

Copied from nsBaseClipboard.cpp
Removed mEmptyingForSetData because only widget/windows/nsClipboard.cpp seems to be checking for it.
https://dxr.mozilla.org/mozilla-central/search?q=mEmptyingForSetData&redirect=false

Comment 133

[Jim Mathies [:jimm]]

Comment on attachment 8780275 [details]
Bug 1261299 - Implement nsIClipboard methods in nsClipboard.mm with methods specific for osx. (EmptyClipboard, SetData) methods modified from nsBaseClipboard to also set the current selection cache.

https://reviewboard.mozilla.org/r/70972/#review69422

::: widget/cocoa/nsClipboard.mm:690
(Diff revision 4)
> +  */
> +NS_IMETHODIMP
> +nsClipboard::SetData(nsITransferable* aTransferable, nsIClipboardOwner* anOwner,
> +                     int32_t aWhichClipboard)
> +{
> +  NS_ASSERTION ( aTransferable, "clipboard given a null transferable" );

nit - remove (*X*) spacing from this and other paren in the patch.

::: widget/cocoa/nsClipboard.mm:700
(Diff revision 4)
> +      return NS_OK;
> +    }
> +    return NS_ERROR_FAILURE;
> +  }
> +
> +  if (aTransferable == mTransferable && anOwner == mClipboardOwner)

nit - brace this and other single line if blocks

Comment 134

[Jimmy Wang (:jimicy) - works on e10s stuff]

Comment on attachment 8769244 [details]
Bug 1261299 - Add new clipboard kSelectionCache to cache the current selection for OSX service menu. Add a constructor to nsAutoCopyListener which sets the clipboard to copy to. Pass in kSelectionCache for OSX or kSelectionClipboard for linux.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63202/diff/7-8/

Comment 135

[Jimmy Wang (:jimicy) - works on e10s stuff]

Comment on attachment 8769245 [details]
Bug 1261299 - Add sSelectionTransferable and to get the current selection (chrome/content) needed for the OSX service menu.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63204/diff/7-8/

Comment 136

[Jimmy Wang (:jimicy) - works on e10s stuff]

Comment on attachment 8769246 [details]
Bug 1261299 - Add method nsCopySupport::ClearSelectionTransferable() to clear the nsClipboard::sSelectionTransferable when you have no content selection.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63206/diff/7-8/

Comment 137

[Jimmy Wang (:jimicy) - works on e10s stuff]

Comment on attachment 8769250 [details]
Bug 1261299 - On e10s in nsChildView for OSX services, determine if there is a current selection (chrome/content). Send the selection to osx service menu.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63214/diff/9-10/

Comment 138

[Jimmy Wang (:jimicy) - works on e10s stuff]

Comment on attachment 8770813 [details]
Bug 1261299 - Add EnsureDocument(mPresContext) to EventStateManager::GetFocusedContent() because mDocument is lazily loaded.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64144/diff/7-8/

Comment 139

[Jimmy Wang (:jimicy) - works on e10s stuff]

Comment on attachment 8775335 [details]
Bug 1261299 - Update the selection cache on repaint to handle when a pre-existing selection becomes active aka the current selection.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67480/diff/5-6/

Comment 140

[Jimmy Wang (:jimicy) - works on e10s stuff]

Comment on attachment 8775590 [details]
Bug 1261299 - Only update nsClipboard::sSelectionCache in nsSelection.cpp when the selection listener is on the same process as the focused window where the selection occured or became active.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67754/diff/5-6/

Comment 141

[Jimmy Wang (:jimicy) - works on e10s stuff]

Comment on attachment 8769248 [details]
Bug 1261299 - Add testing method GetSelectionAsPlaintext to DomWindowUtils which returns the text selection that was sent to the osx service menu nsChildView.mm.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63210/diff/11-12/

Comment 142

[Jimmy Wang (:jimicy) - works on e10s stuff]

Comment on attachment 8769249 [details]
Bug 1261299 - Add test for checking that the selected content/chrome text is properly sent up to the osx service menu code (nsChildView.mm).

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63212/diff/11-12/

Comment 143

[Jimmy Wang (:jimicy) - works on e10s stuff]

Comment on attachment 8780275 [details]
Bug 1261299 - Implement nsIClipboard methods in nsClipboard.mm with methods specific for osx. (EmptyClipboard, SetData) methods modified from nsBaseClipboard to also set the current selection cache.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/70972/diff/4-5/

Comment 144

[Jimmy Wang (:jimicy) - works on e10s stuff]

Comment on attachment 8769244 [details]
Bug 1261299 - Add new clipboard kSelectionCache to cache the current selection for OSX service menu. Add a constructor to nsAutoCopyListener which sets the clipboard to copy to. Pass in kSelectionCache for OSX or kSelectionClipboard for linux.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63202/diff/8-9/

Comment 145

[Jimmy Wang (:jimicy) - works on e10s stuff]

Comment on attachment 8769245 [details]
Bug 1261299 - Add sSelectionTransferable and to get the current selection (chrome/content) needed for the OSX service menu.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63204/diff/8-9/

Comment 146

[Jimmy Wang (:jimicy) - works on e10s stuff]

Comment on attachment 8769246 [details]
Bug 1261299 - Add method nsCopySupport::ClearSelectionTransferable() to clear the nsClipboard::sSelectionTransferable when you have no content selection.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63206/diff/8-9/

Comment 147

[Jimmy Wang (:jimicy) - works on e10s stuff]

Comment on attachment 8769250 [details]
Bug 1261299 - On e10s in nsChildView for OSX services, determine if there is a current selection (chrome/content). Send the selection to osx service menu.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63214/diff/10-11/

Comment 148

[Jimmy Wang (:jimicy) - works on e10s stuff]

Comment on attachment 8770813 [details]
Bug 1261299 - Add EnsureDocument(mPresContext) to EventStateManager::GetFocusedContent() because mDocument is lazily loaded.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64144/diff/8-9/

Comment 149

[Jimmy Wang (:jimicy) - works on e10s stuff]

Comment on attachment 8775335 [details]
Bug 1261299 - Update the selection cache on repaint to handle when a pre-existing selection becomes active aka the current selection.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67480/diff/6-7/

Comment 150

[Jimmy Wang (:jimicy) - works on e10s stuff]

Comment on attachment 8775590 [details]
Bug 1261299 - Only update nsClipboard::sSelectionCache in nsSelection.cpp when the selection listener is on the same process as the focused window where the selection occured or became active.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67754/diff/6-7/

Comment 151

[Jimmy Wang (:jimicy) - works on e10s stuff]

Comment on attachment 8769248 [details]
Bug 1261299 - Add testing method GetSelectionAsPlaintext to DomWindowUtils which returns the text selection that was sent to the osx service menu nsChildView.mm.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63210/diff/12-13/

Comment 152

[Jimmy Wang (:jimicy) - works on e10s stuff]

Comment on attachment 8769249 [details]
Bug 1261299 - Add test for checking that the selected content/chrome text is properly sent up to the osx service menu code (nsChildView.mm).

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63212/diff/12-13/

Comment 153

[Jimmy Wang (:jimicy) - works on e10s stuff]

Comment on attachment 8780275 [details]
Bug 1261299 - Implement nsIClipboard methods in nsClipboard.mm with methods specific for osx. (EmptyClipboard, SetData) methods modified from nsBaseClipboard to also set the current selection cache.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/70972/diff/5-6/

Comment 154

[Jimmy Wang (:jimicy) - works on e10s stuff]

Green try push
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b3aa608e5cd1&selectedJob=25833806

Comment 155

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/fc0cbef7e3e9
Add new clipboard kSelectionCache to cache the current selection for OSX service menu. Add a constructor to nsAutoCopyListener which sets the clipboard to copy to. Pass in kSelectionCache for OSX or kSelectionClipboard for linux. r=mstange
https://hg.mozilla.org/integration/autoland/rev/fb83488b8880
Add sSelectionTransferable and to get the current selection (chrome/content) needed for the OSX service menu. r=mstange
https://hg.mozilla.org/integration/autoland/rev/6888ab7ba2f3
Add method nsCopySupport::ClearSelectionTransferable() to clear the nsClipboard::sSelectionTransferable when you have no content selection. r=mstange
https://hg.mozilla.org/integration/autoland/rev/02eede58003b
On e10s in nsChildView for OSX services, determine if there is a current selection (chrome/content). Send the selection to osx service menu. r=masayuki
https://hg.mozilla.org/integration/autoland/rev/f6a4a9f03907
Add EnsureDocument(mPresContext) to EventStateManager::GetFocusedContent() because mDocument is lazily loaded. r=smaug
https://hg.mozilla.org/integration/autoland/rev/959b3840084a
Update the selection cache on repaint to handle when a pre-existing selection becomes active aka the current selection. r=smaug
https://hg.mozilla.org/integration/autoland/rev/0ea81a4d9c2f
Only update nsClipboard::sSelectionCache in nsSelection.cpp when the selection listener is on the same process as the focused window where the selection occured or became active. r=smaug
https://hg.mozilla.org/integration/autoland/rev/02b3fb6a0d0e
Add testing method GetSelectionAsPlaintext to DomWindowUtils which returns the text selection that was sent to the osx service menu nsChildView.mm. r=masayuki
https://hg.mozilla.org/integration/autoland/rev/f4a840250ccb
Add test for checking that the selected content/chrome text is properly sent up to the osx service menu code (nsChildView.mm). r=masayuki
https://hg.mozilla.org/integration/autoland/rev/623c48bf36a4
Implement nsIClipboard methods in nsClipboard.mm with methods specific for osx. (EmptyClipboard, SetData) methods modified from nsBaseClipboard to also set the current selection cache. r=jimm

Comment 156

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/fc0cbef7e3e9
https://hg.mozilla.org/mozilla-central/rev/fb83488b8880
https://hg.mozilla.org/mozilla-central/rev/6888ab7ba2f3
https://hg.mozilla.org/mozilla-central/rev/02eede58003b
https://hg.mozilla.org/mozilla-central/rev/f6a4a9f03907
https://hg.mozilla.org/mozilla-central/rev/959b3840084a
https://hg.mozilla.org/mozilla-central/rev/0ea81a4d9c2f
https://hg.mozilla.org/mozilla-central/rev/02b3fb6a0d0e
https://hg.mozilla.org/mozilla-central/rev/f4a840250ccb
https://hg.mozilla.org/mozilla-central/rev/623c48bf36a4

Comment 157

[steve]

Still waiting for this to show in the nightly build of FF. Now that this is fixed, are there plans to address https://bugzilla.mozilla.org/show_bug.cgi?id=670457 and specifically https://bugzilla.mozilla.org/show_bug.cgi?id=1284890 ?

Comment 158

[steve]

Fix verified, thanks a lot. Will this be brought to earlier FF versions than 51? 50 seemed to be the first release with e10l defaulted to "on" for all users. If so, this would be highly welcome in v50, since otherwise services would be broken in version 50. But I might be missing something, so bear with me.

Comment 159

[David Durst [:ddurst]]

(In reply to steve-_- from comment #158)
> Fix verified, thanks a lot. Will this be brought to earlier FF versions than
> 51? 50 seemed to be the first release with e10l defaulted to "on" for all
> users. If so, this would be highly welcome in v50, since otherwise services
> would be broken in version 50. But I might be missing something, so bear
> with me.

This sounds like a question for Ritu and/or Andrew. Do you think this is safe to lift to 50?

Comment 160

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

(In reply to David Durst [:ddurst] from comment #159)
> (In reply to steve-_- from comment #158)
> > Fix verified, thanks a lot. Will this be brought to earlier FF versions than
> > 51? 50 seemed to be the first release with e10l defaulted to "on" for all
> > users. If so, this would be highly welcome in v50, since otherwise services
> > would be broken in version 50. But I might be missing something, so bear
> > with me.
> 
> This sounds like a question for Ritu and/or Andrew. Do you think this is
> safe to lift to 50?

I am not aware of any plans to ship this in Fx50. Just going by the # of patches in this bug and the fact that 50 is a week away from entering Beta cycle, I would hesitate to uplift this to Fx50. Perhaps Brad, Jimm can review the patches and share their risk assessment. It seems best to let this one ride the 51 train.

Comment 161

[Brad Lassey [:blassey] (use needinfo?)]

The assumption from earlier in the bug is that this won't be hit by many users and if that is true, this doesn't make a lot of sense to uplift.

Comment 162

[Jim Mathies [:jimm]]

this should ride.